feat(native-app): Revert "feat: update vehicleList endpoint to V2" #16823
Conversation
This reverts commit 036f952.
WalkthroughThe pull request introduces significant changes to the GraphQL fragments and queries related to vehicle data. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
apps/native/app/src/graphql/fragments/vehicle.fragment.graphql (2)
37-38: Field naming consistency improvementThe field
requiresMileageRegistrationis used alongsidecanRegisterMileage. For consistency in naming:Consider renaming to maintain consistent terminology:
requiresMileageRegistration - canRegisterMileage + canRegisterMileageRegistration
15-23: Consider grouping related fieldsThe current field ordering mixes different concerns. Consider grouping related fields together for better maintainability:
Suggested grouping:
- Basic vehicle info (permno, regno, vin, type, etc.)
- Registration details (firstRegDate, registrationType, etc.)
- Status fields (outOfUse, vehicleStatus, plateStatus)
- Ownership fields (ownerPersidno, buyerPersidno, etc.)
- Inspection fields (all inspection-related fields)
- Mileage registration fields
This would make the fragment more organized and easier to maintain.
apps/native/app/src/graphql/queries/vehicles.graphql (1)
Line range hint
1-11: Consider maintaining API versioning strategyThe removal of the V2 suffix from the query name might impact the API versioning strategy. While this is a temporary revert, consider:
- Using a feature flag to toggle between V1 and V2 implementations
- Maintaining both versions until the next release
- Documenting the temporary nature of this revert in the schema
This approach would provide a smoother transition when re-implementing the V2 endpoint in two weeks.
apps/native/app/src/screens/home/vehicles-module.tsx (2)
173-173: Consider updating related documentationThe export statement correctly reflects the reversion to V1. Consider updating any related documentation or API references that might mention the V2 endpoint.
Line range hint
1-173: Consider version management strategy for the upcoming re-implementationSince this reversion is temporary and will be re-implemented in two weeks:
- Consider adding TODO comments or creating tracking issues to ensure all V1 references are updated in the future PR
- Document the typo issue to prevent similar problems in future API updates
- Consider implementing version compatibility layer to handle such transitions more smoothly in the future
Would you like me to help create a tracking issue for the re-implementation?
apps/native/app/src/screens/vehicles/vehicles.tsx (1)
Line range hint
187-207: Simplify pagination logic and improve readabilityThe pagination logic could be improved in the following ways:
- const pageNumber = res.data?.vehiclesList?.paging?.pageNumber ?? 1 - const totalPages = res.data?.vehiclesList?.paging?.totalPages ?? 1 + const paging = res.data?.vehiclesList?.paging + const pageNumber = paging?.pageNumber ?? 1 + const totalPages = paging?.totalPages ?? 1 // ... - vehiclesList: { - ...fetchMoreResult.vehiclesList, - vehicleList: [ - ...(prev.vehiclesList?.vehicleList ?? []), - ...(fetchMoreResult.vehiclesList?.vehicleList ?? []), - ], - }, + vehiclesList: { + ...fetchMoreResult.vehiclesList, + vehicleList: [ + ...(prev.vehiclesList?.vehicleList ?? []), + ...(fetchMoreResult.vehiclesList?.vehicleList ?? []), + ].filter(Boolean), + },These changes:
- Reduce repetitive optional chaining
- Add null filtering to ensure data integrity
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
apps/native/app/src/graphql/fragments/vehicle.fragment.graphql(1 hunks)apps/native/app/src/graphql/queries/vehicles.graphql(1 hunks)apps/native/app/src/screens/home/home.tsx(2 hunks)apps/native/app/src/screens/home/vehicles-module.tsx(5 hunks)apps/native/app/src/screens/vehicles/components/vehicle-item.tsx(3 hunks)apps/native/app/src/screens/vehicles/vehicles.tsx(8 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/native/app/src/graphql/fragments/vehicle.fragment.graphql (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/graphql/queries/vehicles.graphql (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/screens/home/home.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/screens/home/vehicles-module.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/screens/vehicles/components/vehicle-item.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/screens/vehicles/vehicles.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (11)
apps/native/app/src/screens/vehicles/components/vehicle-item.tsx (3)
6-6: LGTM: Type changes align with V2 endpoint reversion
The changes correctly revert the GraphQL query types while maintaining type safety.
Also applies to: 14-14
54-54: Verify the user experience impact of display changes
The vehicle identification has changed from using make to type, which might affect how users identify vehicles. Please confirm:
- Does
typeprovide sufficient vehicle identification for users? - Is
colorequivalent to the previouscolorNamein terms of user understanding?
Consider adding a more descriptive vehicle identifier, perhaps combining type with another field, to ensure clear vehicle identification for users.
Also applies to: 60-61
34-35: Verify schema structure for inspection date
The nested structure for accessing inspection date has changed. Let's verify this matches the GraphQL schema.
apps/native/app/src/screens/home/vehicles-module.tsx (3)
Line range hint 33-46: LGTM! Data validation logic preserved
The validation function maintains its original logic while correctly updating the type signature and data access path.
62-62: LGTM! Clean data access path update
The data access path has been correctly updated while preserving the component's existing functionality.
20-21: Verify consistent type usage across the codebase
The reversion from V2 to V1 types needs to be consistently applied across all components using these types.
✅ Verification successful
Consistent type usage verified
No lingering V2 references found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining V2 references that might have been missed
rg "ListVehiclesV2|useListVehiclesV2" --type ts
Length of output: 49
apps/native/app/src/screens/vehicles/vehicles.tsx (3)
Line range hint 80-84: LGTM: Query usage is correct
The query implementation is clean and properly configured with the input variables.
17-18: Verify the impact of reverting V2 types
The changes from V2 to non-V2 versions align with the PR objective. However, we should verify that all dependent components are updated accordingly.
#!/bin/bash
# Search for any remaining V2 references that might need updating
rg -l "ListVehiclesV2|vehiclesListV2" apps/native/app/src/Also applies to: 35-35
138-138: Verify the uniqueness of VIN as a key
The key extractor now uses item.vin instead of item.permno. While VINs are typically unique, please ensure that:
- VIN is never null or undefined
- The combination of VIN and index provides a unique key for all scenarios
#!/bin/bash
# Search for VIN handling in the codebase
ast-grep --pattern 'item.vin' Also applies to: 150-150
apps/native/app/src/screens/home/home.tsx (2)
58-58: LGTM: Import statement correctly reverted
The change correctly reverts the import statement from V2 to the original version, aligning with the PR objective.
177-183: Verify the impact of new query parameters
While the reversion of the query hook is correct, please verify:
- That these parameters (
showDeregeristeredandshowHistory) existed in the original version - The UI implications of setting both parameters to
falseif the app was previously showing deregistered or historical vehicles
✅ Verification successful
Parameters showDeregeristered and showHistory verified
The parameters exist and are correctly set to false. Ensure that the UI updates appropriately to handle the absence of deregistered or historical vehicles.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if these parameters existed in the original version and their usage
# Search for previous usage of these parameters
rg -B 3 -A 3 'showDeregeristered|showHistory' --type ts
# Check the GraphQL schema for these parameters
ast-grep --pattern 'input GetVehiclesForUserInput {
$$$
showDeregeristered: $_
showHistory: $_
$$$
}'
Length of output: 19591
This reverts commit 036f952.
What
Reverting since there is a typo in one prop from the api: https://github.com/island-is/island.is/blob/main/libs/api/domains/vehicles/src/lib/models/usersVehicles.model.ts#L235
Why
@thordurhhh will fix before next release so to prevent them needing to make sure to always returning both old and new prop I'm postponing releasing this until after next release (in 2 weeks).
Creating a new PR with this and keeping it open until release has been deployed.
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor