Conversation
|
Thank you for following naming conventions! 😻 |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive booking feature for the car rental application, including vehicle selection, booking creation, payment integration with VnPay, and booking management. The implementation follows Angular v20 patterns with signals, standalone components, and modern control flow syntax.
- Adds complete booking flow with stepper UI (vehicle selection → checkout → payment → confirmation)
- Integrates VnPay payment gateway for processing rental payments
- Implements booking draft persistence using session storage
- Adds booking list and detail views for renters to track their bookings
Reviewed Changes
Copilot reviewed 32 out of 34 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
booking.routes.ts |
Reorganizes routes to separate cars and bookings paths with detail views |
layout.ts |
Updates navigation links to point to new /cars and /bookings routes |
booking-storage.service.ts |
New service for persisting booking drafts in session storage |
payment.ts |
New component handling VnPay payment return and success/failure display |
checkout.ts |
Major rewrite implementing booking confirmation and payment initiation |
cars-page.ts |
New stepper container orchestrating the booking flow across multiple steps |
car-detail.ts |
Enhanced with date/time validation, past date checking, and booking data emission |
booking-detail.ts |
New component displaying booking details with QR code for verification |
booking.ts (BookingsPage) |
New component listing renter's bookings grouped by status |
car-list.ts |
Adds filtering for available vehicles only |
session.service.ts |
New service wrapping sessionStorage with JSON serialization support |
payments.service.ts |
New service for VnPay payment creation and return handling |
bookings.service.ts |
Adds methods for creating bookings and fetching renter bookings |
user.service.ts |
Adds renterId signal for tracking current renter |
auth.service.ts |
Extracts and stores renterId from JWT token |
package.json |
Adds angularx-qrcode dependency for QR code generation |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
src/app/features/customer/booking/pages/cars-page/cars-page.ts:1
- [nitpick] Chaining multiple method calls in template event binding
(nextStep)=\"markPaymentStarted(); goToNextStep()\"reduces readability and testability. Consider creating a single method likeonCheckoutComplete()that calls both methods in sequence.
import {
| @Component({ | ||
| selector: 'app-checkout', | ||
| imports: [], | ||
| standalone: true, |
There was a problem hiding this comment.
Remove standalone: true from component metadata. According to the coding guidelines, Angular v20 defaults to standalone components, so this property should never be explicitly added.
| standalone: true, |
| const id = this.bookingId(); | ||
| if (id) { | ||
| try { | ||
| sessionStorage.setItem('lastBookingId', id); |
There was a problem hiding this comment.
Direct sessionStorage access bypasses the SessionService abstraction. Use inject(SessionService).setItem('lastBookingId', id) instead to maintain consistent error handling and SSR compatibility.
| readonly vehicleName = computed(() => { | ||
| const vehicle = this.bookingData()?.vehicle; | ||
| if (!vehicle) return ''; | ||
|
|
||
| const parts: string[] = []; | ||
| if (vehicle.make) parts.push(vehicle.make); | ||
| if (vehicle.model) parts.push(vehicle.model); | ||
| return parts.length ? parts.join(' ') : 'Xe chưa xác định'; | ||
| }); |
There was a problem hiding this comment.
The vehicleName computed logic is duplicated in both Checkout (lines 70-78) and BookingDetail (lines 70-78). Extract this into a shared utility function or method to reduce duplication and ensure consistent vehicle name formatting across components.
| } | ||
| }, | ||
| }); | ||
| this.destroyRef.onDestroy(() => renderRef.destroy()); |
There was a problem hiding this comment.
Registering a destroy callback inside setActiveStep (which can be called multiple times) will leak memory as each call creates a new destroy listener that never gets cleaned up until component destruction. The afterNextRender callback is automatically cleaned up, so the manual destroyRef.onDestroy registration on line 229 should be removed.
| this.destroyRef.onDestroy(() => renderRef.destroy()); |
| @for (metric of vehicleMetrics(); track metric.label) { | ||
| <div | ||
| class="car-detail__metric flex items-center gap-4 px-4 py-4 first:pt-0 last:pb-0 hover:bg-surface-container-high transition-colors" | ||
| class="car-detail__metric flex items-center gap-4 px-4 py-4 hover:bg-surface-container-high transition-colors" |
There was a problem hiding this comment.
[nitpick] Removed first:pt-0 last:pb-0 utility classes. If the first/last item padding adjustment was intentional for visual alignment, this change may affect the layout. Consider whether the padding should be restored or if the parent container's padding should be adjusted instead.
| class="car-detail__metric flex items-center gap-4 px-4 py-4 hover:bg-surface-container-high transition-colors" | |
| class="car-detail__metric flex items-center gap-4 px-4 py-4 first:pt-0 last:pb-0 hover:bg-surface-container-high transition-colors" |
| getRental(rentalId: string): Observable<RentalDetailsDto> { | ||
| return this._rentalService | ||
| .apiRentalRentalIdGet(rentalId) | ||
| .pipe( | ||
| map((response: RentalDetailsDtoApiResponse) => response.data ?? ({} as RentalDetailsDto)), |
There was a problem hiding this comment.
Returning an empty object {} as RentalDetailsDto when response.data is null/undefined can lead to unexpected behavior in consuming code. Consider throwing an error or returning null to make the absence of data explicit, forcing callers to handle the error case properly.
| getRental(rentalId: string): Observable<RentalDetailsDto> { | |
| return this._rentalService | |
| .apiRentalRentalIdGet(rentalId) | |
| .pipe( | |
| map((response: RentalDetailsDtoApiResponse) => response.data ?? ({} as RentalDetailsDto)), | |
| getRental(rentalId: string): Observable<RentalDetailsDto | null> { | |
| return this._rentalService | |
| .apiRentalRentalIdGet(rentalId) | |
| .pipe( | |
| map((response: RentalDetailsDtoApiResponse) => response.data ?? null), |
| getRenterProfile(renterId: string): Observable<RenterProfileDto> { | ||
| return this._bookingService | ||
| .apiBookingRenterProfileGet(renterId) | ||
| .pipe( | ||
| map((response: RenterProfileDtoApiResponse) => response.data ?? ({} as RenterProfileDto)), |
There was a problem hiding this comment.
Returning an empty object {} as RenterProfileDto when response.data is null/undefined can lead to unexpected behavior. Consider throwing an error or returning null to make the absence of data explicit, forcing callers to handle the error case.
| getRenterProfile(renterId: string): Observable<RenterProfileDto> { | |
| return this._bookingService | |
| .apiBookingRenterProfileGet(renterId) | |
| .pipe( | |
| map((response: RenterProfileDtoApiResponse) => response.data ?? ({} as RenterProfileDto)), | |
| getRenterProfile(renterId: string): Observable<RenterProfileDto | null> { | |
| return this._bookingService | |
| .apiBookingRenterProfileGet(renterId) | |
| .pipe( | |
| map((response: RenterProfileDtoApiResponse) => response.data ?? null), |
| setTimeout(() => { | ||
| window.location.href = paymentUrl; | ||
| }, 100); |
There was a problem hiding this comment.
Using a magic number timeout of 100ms is fragile and may not be sufficient on slower devices. The comment on line 209 suggests this is to allow parent state updates, but a better approach would be to use afterNextRender to ensure the UI has updated before navigation, or emit the event and let the parent handle the redirect.
| private readonly destroyRef = inject(DestroyRef); | ||
|
|
||
| readonly hourOptions = HOUR_OPTIONS; | ||
| readonly today = this.stripTime(new Date()); |
There was a problem hiding this comment.
[nitpick] The today signal is initialized with this.stripTime(new Date()) which will be stale if the component lives past midnight. While unlikely to cause issues in a booking flow, consider using a computed signal that regenerates the date value, or document this limitation if the current behavior is acceptable.
| readonly today = this.stripTime(new Date()); | |
| // Signal for the current time, updated at midnight to ensure 'today' is always correct | |
| private readonly now = signal(new Date()); | |
| // Effect to update 'now' at the next midnight (and every midnight thereafter) | |
| private readonly _midnightUpdater = effect(() => { | |
| const now = new Date(); | |
| const nextMidnight = new Date(now.getFullYear(), now.getMonth(), now.getDate() + 1, 0, 0, 0, 0); | |
| const msUntilMidnight = nextMidnight.getTime() - now.getTime(); | |
| const timeout = setTimeout(() => { | |
| this.now.set(new Date()); | |
| }, msUntilMidnight); | |
| return () => clearTimeout(timeout); | |
| }); | |
| readonly today = computed(() => this.stripTime(this.now())); |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Handle payment return only; ignore external step changes via URL | ||
| this.route.queryParams.pipe(takeUntilDestroyed(this.destroyRef)).subscribe((params) => { | ||
| // Check if this is a payment return (VnPay usually returns with vnp_* params) | ||
| const hasPaymentParams = Object.keys(params).some((key) => key.startsWith('vnp_')); | ||
| if (hasPaymentParams) { | ||
| this.handlePaymentReturn(); | ||
| } | ||
| }); | ||
|
|
||
| this.route.fragment.pipe(takeUntilDestroyed(this.destroyRef)).subscribe((fragment) => { | ||
| const normalized = fragment?.trim().toLowerCase(); | ||
| if (!normalized) { | ||
| return; | ||
| } | ||
|
|
||
| const targetIndex = this.steps.findIndex((step) => step.key === normalized); | ||
| if (targetIndex < 0 || targetIndex === this.activeStep()) { | ||
| return; | ||
| } | ||
|
|
||
| this.setActiveStep(targetIndex, { updateFragment: false }); | ||
| }); | ||
| } | ||
|
|
||
| private handlePaymentReturn(): void { | ||
| this.paymentsService | ||
| .getPaymentReturn() | ||
| .pipe( |
There was a problem hiding this comment.
When the user is redirected back from VnPay, you detect the presence of vnp_* query parameters but then call paymentsService.getPaymentReturn() without forwarding any of those values. The generated apiPaymentReturnGet() endpoint issues a plain GET to /api/Payment/return; without the signature, amount, order id, etc. from the query string the server cannot validate or even identify the transaction, so the verification call will consistently fail or return empty data even though the client has the required parameters. Pass the current query parameters through to the service so that the backend can verify the payment result.
Useful? React with 👍 / 👎.
No description provided.