Add staff booking workflow detail page#36
Conversation
Deploying ev-rental with
|
| Latest commit: |
4e22cfc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://83c1bb97.ev-rental.pages.dev |
| Branch Preview URL: | https://codex-implement-booking-deta-mwoe.ev-rental.pages.dev |
|
Thank you for following naming conventions! 😻 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a staff booking workflow feature that guides staff through the complete rental lifecycle, from booking check-in to vehicle handover. It introduces a new service to orchestrate sequential API calls and a detailed form-driven UI.
- Introduces
BookingWorkflowServiceto sequence check-in, rental creation, contract creation, inspection, signatures, and vehicle receiving - Adds
StaffBookingDetailComponentwith a comprehensive form to capture workflow inputs and display progress - Connects the staff dashboard to the new workflow page via a "Continue workflow" button and new routing
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
staff.routes.ts |
Adds route for the booking detail page with dynamic bookingId parameter |
staff-dashboard.ts |
Adds navigation method and Router injection to support workflow navigation |
staff-dashboard.scss |
Adds styling for the new action button in the details panel |
staff-dashboard.html |
Adds "Continue workflow" button in the details panel footer |
booking-detail.ts |
Implements comprehensive workflow component with reactive forms and state management |
booking-detail.scss |
Provides complete styling for the workflow UI with responsive design |
booking-detail.html |
Creates guided form interface with validation, progress tracking, and results display |
booking-workflow.service.ts |
Orchestrates sequential API calls with error handling and step tracking |
| effect(() => { | ||
| const record = this.booking(); | ||
| if (record) { | ||
| this._prefillForm(record); |
There was a problem hiding this comment.
This effect will execute on every change detection cycle whenever the booking signal changes, causing the form to be repeatedly patched. This can override user edits if the booking data updates. Consider using { allowSignalWrites: false } option or add logic to only prefill when the booking ID changes (not on subsequent updates to the same booking).
| effect(() => { | |
| const record = this.booking(); | |
| if (record) { | |
| this._prefillForm(record); | |
| let lastPrefilledBookingId: string | undefined; | |
| effect(() => { | |
| const record = this.booking(); | |
| if (record && record.id !== lastPrefilledBookingId) { | |
| this._prefillForm(record); | |
| lastPrefilledBookingId = record.id; |
| <div class="booking-workflow__alert" role="alert"> | ||
| <mat-icon aria-hidden="true">error</mat-icon> | ||
| <div> | ||
| <h2>Booking not found</h2> |
There was a problem hiding this comment.
The h2 heading inside the alert div breaks the document outline. Since this is an alert message, consider using a <strong> or <p> with appropriate styling instead of a heading element, as the alert already has proper ARIA semantics with role=\"alert\".
| <h2>Booking not found</h2> | |
| <p><strong>Booking not found</strong></p> |
| <li>Rental ID: {{ outcome.rentalId }}</li> | ||
| <li>Contract ID: {{ outcome.contractId }}</li> | ||
| <li>Inspection ID: {{ outcome.inspectionId }}</li> | ||
| <li>Signature IDs: {{ outcome.signatureIds[0] }}, {{ outcome.signatureIds[1] }}</li> |
There was a problem hiding this comment.
Direct array indexing without bounds checking could cause runtime errors if the signatures array doesn't contain exactly 2 elements. While the type is readonly [string, string], consider using safer access or a conditional check before rendering.
| <li>Signature IDs: {{ outcome.signatureIds[0] }}, {{ outcome.signatureIds[1] }}</li> | |
| @if (outcome.signatureIds?.length === 2) { | |
| <li>Signature IDs: {{ outcome.signatureIds[0] }}, {{ outcome.signatureIds[1] }}</li> | |
| } @else { | |
| <li>Signature IDs: —</li> | |
| } |
| currentBatteryCapacityKwh: payload.inspection.currentBatteryCapacityKwh, | ||
| inspectedAt: payload.inspection.inspectedAt, | ||
| inspectorStaffId: payload.inspection.inspectorStaffId, | ||
| url: payload.inspection.url ?? null, |
There was a problem hiding this comment.
The inspection URL is converted from string | undefined to string | null. This inconsistency could cause issues since the OpenAPI contract might expect one format over the other. Verify that the API accepts null for optional string fields, or keep it as undefined.
| url: payload.inspection.url ?? null, | |
| url: payload.inspection.url, |
| finalize(() => { | ||
| this._loading.set(false); | ||
| if (this._step() !== 'completed') { | ||
| this._step.set('idle'); |
There was a problem hiding this comment.
In the error case, the step is reset to 'idle' only if not completed, but the error state persists. This creates an inconsistent state where error is set but step is 'idle'. Consider either keeping the failed step visible or clearing the error when resetting to 'idle' to maintain state consistency.
| this._step.set('idle'); | |
| this._step.set('idle'); | |
| this._error.set(null); |
| this.toastService.success('Booking workflow completed successfully.'); | ||
| }, | ||
| error: (error) => { | ||
| console.error('Booking workflow failed', error); |
There was a problem hiding this comment.
Console logging in production code should be avoided or wrapped in a proper logging service. Consider removing this or routing through a centralized logging mechanism that can be controlled via environment configuration.
| console.error('Booking workflow failed', error); | |
| // Logging removed to comply with production standards. |
Summary
Testing
Codex Task