fix(assets): bulk mileage fixes #16253
Conversation
WalkthroughThe pull request introduces several modifications across multiple files related to vehicle mileage management. Key changes include the addition of a logger dependency in the Changes
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
libs/service-portal/core/src/components/NestedTable/NestedFullTable.tsx (2)
Line range hint
8-13: Enhance type safety for better reusabilityTo improve type safety and make this component more reusable across different NextJS apps, consider using a more specific type for the
dataprop. Instead ofArray<string[]>, you could define a generic type parameter. This would allow consumers of the component to specify the exact shape of their data.Here's a suggestion:
interface Props<T extends string[]> { headerArray: string[] data: T[] loading?: boolean emptyMessage?: string } export const NestedFullTable = <T extends string[]>({ headerArray, data, loading, emptyMessage, }: Props<T>) => { // ... rest of the component }This change would make the component more flexible and type-safe when used in different contexts.
Line range hint
22-69: Add null check for data propTo improve the robustness of the component, consider adding a null check for the
dataprop. This will prevent potential runtime errors ifdatais undefined.Here's a suggestion:
{!loading && data && !!data.length && ( <T.Table> {/* ... existing table code ... */} </T.Table> )} {(loading || !data || !data.length) && ( <EmptyTable loading={loading} message={emptyMessage ?? 'Ekkert fannst'} /> )}This change ensures that the component handles cases where
datamight be undefined, further improving its reusability across different contexts.libs/api/domains/vehicles/src/lib/resolvers/mileage.resolver.ts (1)
45-48: LGTM: Constructor updated with logger injection.The constructor has been correctly updated to inject a logger, which enhances the class's logging capabilities. The use of
@Inject(LOGGER_PROVIDER)and TypeScript typing for the logger is correct and follows NestJS best practices.Consider using a more specific logger name, such as
vehiclesMileageLogger, to improve code readability and maintain consistency across the codebase.libs/service-portal/assets/src/screens/VehicleMileage/VehicleMileage.tsx (2)
252-253: LGTM! Consider adding a type assertion for clarity.The use of optional chaining and a fallback value improves the robustness of the code. Great job!
For improved clarity and type safety, consider adding a type assertion:
const latestRegistration = (detailArray?.[0]?.mileageNumber ?? 0) as number;This explicitly indicates that
latestRegistrationis always a number, which may help prevent type-related issues in the future.
Line range hint
1-424: Enhance type definitions and exports for better adherence to coding guidelines.The file generally follows the coding guidelines for
libs/**/*files. However, consider the following improvements:
Define and export interface for component props to enhance reusability:
export interface VehicleMileageProps { // Add any props here if needed } const VehicleMileage: React.FC<VehicleMileageProps> = () => { // ... component implementation }Export types used in the component to improve reusability across different NextJS apps:
export interface FormData { odometerStatus: number }Consider extracting reusable logic (e.g., mileage validation) into a custom hook for better modularity and reusability.
These changes will further align the component with the coding guidelines, enhancing its reusability and type safety across different NextJS apps.
libs/api/domains/vehicles/src/lib/services/vehicles.service.ts (1)
Line range hint
446-461: LGTM! Consider enhancing error handling.The changes to the
putMileageReadingmethod look good. The return type now correctly reflects that a singleMileageReadingDtois being updated, which aligns better with the method's purpose. The simplified implementation is more concise and easier to understand.Consider adding a try-catch block to handle potential errors from the
rootPutcall:try { return this.getMileageWithAuth(auth).rootPut({ putMileageReadingModel: input, }); } catch (error) { this.logger.error('Error updating mileage reading', { category: LOG_CATEGORY, error, }); throw error; }This would provide better error logging and maintain consistency with error handling patterns used elsewhere in the service.
libs/clients/vehicles-mileage/src/clientConfig.json (1)
Line range hint
1-1005: Summary of changes and potential impactThe modifications to the OpenAPI specification for the SGS Rest API, specifically in the PUT / endpoint response, improve the API's consistency and adherence to RESTful best practices. However, these changes may have implications for existing client applications:
- Clients expecting a 200 status code will need to be updated to handle 201.
- Any code processing the response as an array will need to be modified to handle a single object.
To ensure a smooth transition:
- Update all client applications consuming this API to accommodate these changes.
- Consider versioning the API or providing a deprecation period if immediate updates to all clients are not feasible.
- Update any documentation or API guides to reflect these changes.
Consider implementing API versioning to allow for smoother transitions in future API changes. This could involve including version numbers in the URL path or using custom headers to specify the API version.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- libs/api/domains/vehicles/src/lib/resolvers/mileage.resolver.ts (3 hunks)
- libs/api/domains/vehicles/src/lib/services/vehicles.service.ts (2 hunks)
- libs/clients/vehicles-mileage/src/clientConfig.json (1 hunks)
- libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx (2 hunks)
- libs/service-portal/assets/src/screens/VehicleMileage/VehicleMileage.tsx (1 hunks)
- libs/service-portal/core/src/components/NestedTable/NestedFullTable.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
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/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/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/VehicleMileage/VehicleMileage.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/core/src/components/NestedTable/NestedFullTable.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 (7)
libs/service-portal/core/src/components/NestedTable/NestedFullTable.tsx (1)
22-22: Approved: Improved boolean condition clarityThe change from
data.lengthto!!data.lengthexplicitly converts the length to a boolean value. While this doesn't alter the behavior, it improves code readability by making the intent clearer. This is a good practice in TypeScript/JavaScript.libs/api/domains/vehicles/src/lib/resolvers/mileage.resolver.ts (2)
10-10: LGTM: Import statements are well-structured and follow best practices.The new import statements for
Inject,LOGGER_PROVIDER, andLoggertype are correctly added and follow TypeScript best practices. These specific imports support effective tree-shaking.Also applies to: 37-37
95-97: Simplified condition approved, but clarification needed on return statement change.The simplification of the condition check improves code readability. However, the change in the return statement from
res[0]toresmight affect the method's behavior.Could you please clarify the rationale behind changing
mileageDetailConstructor(res[0])tomileageDetailConstructor(res)? This change might impact the output of the mutation.To verify the impact, let's check the implementation of
mileageDetailConstructor:Also applies to: 99-99
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx (3)
50-50: LGTM: Improved error handling consistencyThis change ensures that the
postSuccessstate is reset tofalsewhen an error occurs during the put action, maintaining consistency with the error handling in the post action. This improvement enhances the overall state management of the component.
62-66: LGTM: Fixed success state managementThese changes improve the state management for the post action:
- Line 62: Adding
setPostSuccess(false)in theonErrorcallback ensures consistent error handling with the put action.- Line 66: Changing
setPostSuccess(false)tosetPostSuccess(true)in theonCompletedcallback fixes a critical logical error. This ensures that the success state accurately reflects the successful completion of the post action.These improvements will lead to more accurate UI feedback and better overall component behavior.
Line range hint
1-324: Overall assessment: Improved state managementThe changes in this file focus on enhancing the state management for success and error handling in the
VehicleBulkMileageRowcomponent. These improvements lead to more accurate UI feedback and better overall component behavior.The modifications align well with the coding guidelines:
- Reusability: The component's structure remains unchanged, maintaining its potential for reuse across different NextJS apps.
- TypeScript usage: The changes don't affect the existing TypeScript implementations, preserving type safety.
- Effective tree-shaking and bundling: No changes were made that would negatively impact bundling practices.
These refinements contribute to a more robust and reliable component without introducing any new issues or compromising existing functionality.
libs/clients/vehicles-mileage/src/clientConfig.json (1)
246-252: Approve the changes to the PUT / endpoint response.The modifications to the PUT / endpoint response are appropriate and align with RESTful API best practices:
- Changing the status code from 200 to 201 is correct for a resource creation operation.
- Updating the response schema from an array to a single object is more consistent with typical PUT operation responses.
These changes improve the API's consistency and adherence to standards.
To ensure these changes don't introduce breaking changes for existing clients, please run the following verification script:
This script will help identify any client code that might need to be updated to handle the new response format.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16253 +/- ##
=======================================
Coverage 36.92% 36.92%
=======================================
Files 6781 6781
Lines 140009 140009
Branches 39810 39810
=======================================
Hits 51703 51703
Misses 88306 88306
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 5 Total Test Services: 0 Failed, 4 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (1)
|
* fix: random 0 in table * fix: fixes --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
...
Attach a link to issue if relevant
What
Specify what you're trying to achieve
Why
Specify why you need to achieve this
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style