fix(skilavottord): PlateCount is not always provided #16233
Conversation
WalkthroughThe pull request introduces modifications to 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: 1
🧹 Outside diff range and nitpick comments (5)
apps/skilavottord/web/screens/DeregisterVehicle/Confirm/Confirm.tsx (5)
103-107: LGTM! Consider using a named constant for the initial plate count.The addition of the
plateCountstate and its handler function aligns well with the PR objective. This change should help address the issue whereplateCountis not always provided.Consider defining the initial plate count as a named constant at the top of the file or in a constants file. This would improve maintainability and make it easier to adjust if needed in the future. For example:
const INITIAL_PLATE_COUNT = 2; // ... const [plateCount, setPlateCount] = useState<number>(INITIAL_PLATE_COUNT)
139-143: LGTM! Consider using optional chaining for improved readability.The modified useEffect hook simplifies the control flow and aligns well with React best practices. It correctly sets the
vehicleReadyToDeregisteredQueryCompletedstate based on the query result.Consider using optional chaining to simplify the condition and improve readability:
useEffect(() => { if (data?.skilavottordVehicleReadyToDeregistered) { setVehicleReadyToDeregisteredQueryCompleted(true) } }, [data])This change makes the code more concise while maintaining the same functionality.
🧰 Tools
🪛 Biome
[error] 140-140: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
164-173: LGTM! Consider extracting the condition to a named function for improved readability.This useEffect hook appropriately sets the
plateCountto 0 based on specific vehicle traffic conditions. It aligns well with the PR objective of handling cases whereplateCountmight not be provided.To improve readability and maintainability, consider extracting the condition into a named function:
const shouldSetPlateCountToZero = (trafficData) => trafficData && trafficData.outInStatus.toLocaleUpperCase() === 'OUT' && trafficData.useStatus !== UseStatus.OUT_TICKET; useEffect(() => { if (shouldSetPlateCountToZero(vehicleTrafficData)) { setPlateCount(0) } }, [vehicleTrafficData])This change would make the code more self-documenting and easier to understand at a glance.
Line range hint
1-358: Consider refactoring for improved code organization and reusability.The overall structure of the component follows React and NextJS best practices, with appropriate use of hooks and state management. However, there are opportunities for improvement:
- Extract API calls and related logic into custom hooks (e.g.,
useVehicleData,useTrafficData).- Consider creating a separate component for handling loading and error states.
- Extract form-related logic into a custom hook (e.g.,
useDeregistrationForm).These refactoring suggestions could enhance code organization, improve reusability, and make the component easier to test and maintain. For example:
// Custom hook for vehicle data const useVehicleData = (id) => { // ... useQuery logic for SkilavottordVehicleReadyToDeregisteredQuery return { vehicle, loading, error }; }; // Custom hook for traffic data const useTrafficData = (id, shouldFetch) => { // ... useQuery logic for SkilavottordTrafficQuery return { trafficData, loading, error }; }; // In the component const { vehicle, loadingVehicle } = useVehicleData(id); const { trafficData, loadingTraffic } = useTrafficData(id, vehicleReadyToDeregisteredQueryCompleted);Would you like assistance in implementing these refactoring suggestions?
🧰 Tools
🪛 Biome
[error] 140-140: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Line range hint
1-358: Consider leveraging Next.js SSR or SSG features for improved performance.While the component makes good use of Next.js's
useRouterfor client-side routing, there might be opportunities to further optimize the application using Next.js-specific features:
- If the vehicle data doesn't change frequently, consider using
getStaticPropsandgetStaticPathsfor static generation of pages for each vehicle.- For more dynamic data,
getServerSidePropscould be used to fetch initial data on the server, potentially improving initial load time and SEO.Here's an example of how you might implement
getServerSideProps:export async function getServerSideProps(context) { const { id } = context.params; const client = initializeApolloClient(); await client.query({ query: SkilavottordVehicleReadyToDeregisteredQuery, variables: { permno: id }, }); return { props: { initialApolloState: client.cache.extract(), }, }; }This approach could potentially improve the initial page load time and SEO. Would you like assistance in implementing these Next.js-specific optimizations?
🧰 Tools
🪛 Biome
[error] 140-140: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- apps/skilavottord/web/components/CarDetailsBox2/CarDetailsBox2.tsx (4 hunks)
- apps/skilavottord/web/screens/DeregisterVehicle/Confirm/Confirm.tsx (5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/skilavottord/web/components/CarDetailsBox2/CarDetailsBox2.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/skilavottord/web/screens/DeregisterVehicle/Confirm/Confirm.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."
🪛 Biome
apps/skilavottord/web/screens/DeregisterVehicle/Confirm/Confirm.tsx
[error] 140-140: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
apps/skilavottord/web/screens/DeregisterVehicle/Confirm/Confirm.tsx (2)
305-307: LGTM! Props update improves component interaction.The addition of
plateCount,onPlateCountChange, andisLoadingprops to the CarDetailsBox2 component enhances its functionality and user feedback. This change aligns well with the PR objective of handlingplateCountmore effectively and provides better control over the component's state.The use of
loadingTrafficfor theisLoadingprop is a good practice for indicating the loading state to the user.
350-352: LGTM! Improved user experience by disabling button during loading.Disabling the confirm button when
loadingTrafficis true is an excellent UX improvement. This prevents users from submitting multiple requests while data is being loaded, potentially avoiding race conditions or duplicate submissions.This change aligns well with React and NextJS best practices for handling loading states and enhancing user interaction.
apps/skilavottord/web/components/CarDetailsBox2/CarDetailsBox2.tsx (4)
8-8: Import of LoadingDots component is appropriateThe addition of
LoadingDotsfrom@island.is/island-ui/corealigns with its usage in the loading state of the component.
37-39: New props added to BoxProps are appropriateThe props
plateCount,onPlateCountChange, andisLoadingare correctly added to theBoxPropsinterface, ensuring type safety and proper functionality within the component.
51-53: Props are correctly destructured in the componentDestructuring
onPlateCountChange,plateCount, andisLoadingfrom the component props makes them accessible for use within the component logic.
123-127: Loading state implementation enhances user experienceThe conditional rendering based on
isLoadingto display a loading indicator improves the user interface by providing feedback during data fetching processes.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16233 +/- ##
==========================================
- Coverage 36.93% 36.92% -0.01%
==========================================
Files 6781 6781
Lines 140002 140007 +5
Branches 39809 39857 +48
==========================================
+ Hits 51703 51704 +1
- Misses 88299 88303 +4
Flags with carried forward coverage won't be shown. Click here to find out more. see 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
|
TS-922
What
Fix plateCount missing value
Why
The new plateCount field isn’t always returning a value when deregistering the vehicle.
Sometimes I get this:
And sometimes it seems that no value is being picked up.
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor