Repository navigation
Conversation
# Conflicts: # libs/bookings/src/test/booking.utilities.spec.ts # libs/common/src/lib/types/event.class.ts # libs/events/src/tests/event.class.spec.ts
# Conflicts: # libs/events/src/tests/event.class.spec.ts
# Conflicts: # apps/workplace/src/app/book/parking-request-flow/parking-request-form-details.component.ts
# Conflicts: # libs/bookings/src/lib/desk-select-modal/desk-select-modal.component.ts # libs/bookings/src/lib/locker-select-modal/locker-select-modal.component.ts # libs/bookings/src/lib/new-desk-select-modal/new-desk-select-modal.component.ts # libs/bookings/src/lib/new-locker-select-modal/new-locker-select-modal.component.ts # libs/bookings/src/lib/new-parking-select-modal/new-parking-select-modal.component.ts # libs/bookings/src/lib/parking-select-modal/parking-select-modal.component.ts # libs/bookings/src/lib/parking-space-list-field.component.ts # libs/bookings/src/test/booking-form.service.spec.ts # libs/events/src/lib/event-form.service.ts # libs/events/src/lib/new-event-form.service.ts # libs/events/src/lib/new-space-select-modal/new-space-list.component.ts # libs/events/src/lib/new-space-select-modal/new-space-location-pin.component.ts # libs/events/src/lib/new-space-select-modal/new-space-select-modal.component.ts # libs/events/src/lib/space-select-modal/space-select-modal.component.ts # libs/events/src/tests/space-select-modal/space-select-modal.component.spec.ts
# Conflicts: # libs/common/src/lib/recurrence.ts
# Conflicts: # apps/workplace/src/app/schedule/schedule-state.service.ts # libs/bookings/src/test/invite-visitor-form.component.spec.ts # libs/events/src/lib/spaces.service.ts
# Conflicts: # shared/styles/application.css
# Conflicts: # bun.lock # package.json
# Conflicts: # apps/concierge/e2e/lockers.spec.ts # apps/outlook-addin/src/app/rooms/room-confirm.service.ts # apps/outlook-addin/src/tests/upcoming-bookings.component.spec.ts # apps/signage-manager/src/app/signage.service.ts # apps/visitor-kiosk/src/tests/checkin/checkin-qr-scan.component.spec.ts # apps/workplace/src/app/book/book.routes.ts # apps/workplace/src/tests/book/visitor-flow.component.spec.ts # apps/workplace/src/tests/explore/explore.component.spec.ts # apps/workplace/src/tests/schedule/schedule-filter-card.component.spec.ts # apps/workplace/src/tests/schedule/schedule-filters.component.spec.ts # apps/workplace/src/tests/schedule/schedule-sidebar.component.spec.ts # apps/workplace/src/tests/schedule/schedule-state.service.spec.ts # apps/workplace/src/tests/schedule/schedule.component.spec.ts # libs/bookings/src/lib/booking-card.component.ts # libs/bookings/src/lib/booking-details-modal.component.ts # libs/bookings/src/test/booking-form.service.spec.ts # libs/bookings/src/test/desk-filters.component.spec.ts # libs/bookings/src/test/parking-select-modal.component.spec.ts # libs/bookings/src/test/parking-space-list-field.component.spec.ts # libs/bookings/src/test/visitor-invite-form.component.spec.ts # libs/catering/src/lib/catering-list-field.component.ts # libs/common/src/lib/org/organisation.service.ts # libs/common/src/tests/organisation-offline-boot.spec.ts # libs/events/src/lib/space-select-modal/space-select-modal.component.ts # libs/events/src/lib/spaces.service.ts # libs/events/src/tests/space-select-modal/space-select-modal.component.spec.ts # libs/explore/src/lib/explore-space-info.component.ts # libs/form-fields/src/tests/date-field.component.spec.ts
The booking details modal still rendered the three-dot menu and check-in button for a cancelled booking even though every action was blocked. Hide the whole actions block when the booking is cancelled. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Cancelling one day of a multi-day parking booking removed it from the Day, Week and List views. The staff API only expands cancelled instances of a recurring series when the `deleted` flag is sent with `include_deleted`, so the schedule query now sends both. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* feat(common): track route navigation with Google Analytics 4 (HIO687AS) * fix(common): respect analytics settings and correct GA4 events --------- Co-authored-by: Alex Sorafumo <alex@yuion.net>
The desk booking map tooltip now shows who booked each desk in the selected time window. The map loads desk bookings for the active level and window, and hides names when `desks.show_users` is false. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
# Conflicts: # apps/workplace/src/app/landing/landing-state.service.ts # apps/workplace/src/app/schedule/schedule.component.ts # apps/workplace/src/tests/landing/landing-state.service.spec.ts # apps/workplace/src/tests/schedule/schedule.component.spec.ts # libs/bookings/src/lib/booking-details-modal.component.ts # libs/bookings/src/lib/booking-form.service.ts
The API client throws the fetch Response for failed requests, so the cancel and end booking toasts showed "[object Response]". Read the response body for a message and fall back to the HTTP status. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
# Conflicts: # apps/concierge/src/app/parking/parking-bookings-list.component.ts # apps/signage-manager/src/tests/signage.service.spec.ts # apps/workplace/src/app/book/desk-flow.component.ts # libs/bookings/src/lib/booking-card.component.ts # libs/bookings/src/lib/booking-form.service.ts # libs/bookings/src/lib/booking.utilities.ts # libs/bookings/src/lib/parking-space-list-field.component.ts # libs/common/src/lib/placeos.service.ts # libs/common/src/lib/types/booking.class.ts # libs/common/src/tests/general.spec.ts # libs/form-fields/src/index.ts # libs/form-fields/src/lib/date-range-calendar.component.ts # shared/assets/locale/en-AU.json
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
MrYuion
left a comment
There was a problem hiding this comment.
Review of head 597bdd67e against develop, from the merge-base diff of 263 files. I read the code. I did not run the apps. CI builds and tests are green on this head.
There are 38 inline comments, most severe first: 11 high, 20 medium, 7 low.
Merge conflicts
develop is 4 commits ahead of the merge base. Those commits are the lint and any cleanup. A test merge has conflicts in 9 files:
apps/concierge/src/app/parking/parking-bookings-list.component.tsapps/concierge/src/tests/parking/parking-bookings-list.component.spec.tsapps/workplace/src/app/book/parking-request-flow/parking-request-form-details.component.tsapps/workplace/src/app/schedule/schedule-state.service.tsapps/workplace/src/app/schedule/schedule.component.tsapps/workplace/src/tests/schedule/schedule.component.spec.tslibs/bookings/src/lib/booking-form.service.tslibs/bookings/src/test/invite-visitor-form.component.spec.tslibs/events/src/tests/space-select-modal/space-select-modal.component.spec.ts
Main risk
The PR replaces the default landing page, desk flow, meeting flow, visitor flow and schedule page. There is no setting to select the old ones. Two things follow from that.
- The new pages do not read many settings that existing deployments use. The inline comments on
book.routes.tsandapp.routes.tslist them. - Values that belong to one deployment move into the shared defaults: the GA4 measurement ID, the colour palette, the parking shift presets and P2 text, and the virtual concierge image.
I recommend a setting that selects the new UI, or parity with the old flows before this becomes the default. The deployment values belong in zone metadata.
Shared library changes also affect apps outside workplace. The date field, the date range calendar, the capacity filter, the desk filter, the favourite keys and find availability each have an inline comment with an example in Concierge or the Outlook add-in.
Findings without a single line
- Medium. Settings docs are not in sync.
docs/settings/workplace.mdhas no rows forvirtual_concierge.*,virtual_concierge_url,parking.waitlist_week_startandparking.show_waitlist.parking.show_user_groupsis in the Concierge defaults but not in its schema or docs. Six old landing settings are documented and no longer read. - Medium. Tests do not cover the changed submit paths. No spec covers
confirmBookingin the new desk and visitor flows, the Special Needs parking path, orcheckIn()on the home card. The Available Now spec mocks both form services. - Low. Dead code. The old landing page components,
NewDeskFlowComponent,BookMeetingFlowComponent,VisitorFlowComponentand their form components are not reachable from the workplace routes. The PR still edits some of them.schedule-mobile-calendar.component.tsis not used. - Low. Type safety. The diff adds about 70 uses of
anyin non-spec source. Examples:desk-flow-details.component.ts:371,desk-flow-select-map.component.ts:344,meeting-flow-options.component.ts:477,visitor-flow.component.ts:173,landing-upcoming-booking.component.ts:427.develophas just removed these in other files. - Low. Duplicate locale keys. Ten locale files gain a second
REPORTS_ALLOCATIONS,REPORTS_REJECTEDandREPORTS_CANCELLEDin one object. Example:fr.json:1721and:1733. The second value wins, so "Rejetées" becomes "Rejeté". - Low. Hard-coded English. Many strings in
landing-new/, the visitor leave prompt and the parking request summary do not use translation keys.
Checked and found correct
- The
canDeactivateguard on the visitor route. It lets new invites and untouched edits pass, and it does not block the move to the success step. - The virtual concierge button is hidden when no URL is set. The embed URL comes from settings only.
- The new menu items,
team-scheduleincluded, useapp.features. isInWaitlistWeekdate maths, thenth_of_monthrecurrence change,responseErrorMessage, and the analytics opt-out withanalytics.enabled: false.- Schedule polling sends one request for each booking type and stops when the user leaves the page.
- The Playwright specs run in mock mode against
localhost, with no credentials and no customer hosts.
Not verified
- Runtime behaviour. Two comments say "I confirmed this from the code", because they depend on load order.
- Lint against the new rules on
developafter a merge. - Whether the staff API returns
created_atand rejected bookings by default.
Reviewed by an AI agent: Claude Opus 5.5 in Claude Code, run from T3 Code.
| analytics: { | ||
| enabled: true, | ||
| tracking_id: '', | ||
| tracking_id: 'G-S6TDS95BDH', |
There was a problem hiding this comment.
High. A fixed GA4 measurement ID is now the default.
On develop the default was '', so analytics was off until a deployment set an ID. With enabled: true and this ID, each workplace deployment that has no override loads gtag and sends its page views and currentUser().id to this one GA property. _initAnalytics in libs/common/src/lib/placeos.service.ts does this at startup. Concierge gets the same default in apps/concierge/src/environments/settings.ts:15.
Fix: keep tracking_id: '' in both defaults and in the two docs tables. Set the ID in the zone metadata of the deployment that owns it.
| if (new_date < this.from().valueOf()) { | ||
| new_date = this.from().valueOf(); | ||
| } | ||
| // Ensure we don't select a past datetime when switching dates | ||
| if (new_date < Date.now()) { |
There was a problem hiding this comment.
High. The date field can no longer select a past date.
setValue now replaces each value before Date.now() with Date.now(), after the from check. Callers that permit past dates through [from] lose them.
Example: asset-purchase-order-form.component.ts in Concierge sets [from] to five years ago. Select a purchase date in last month and the form stores the current time. Signage time-controls.component.ts uses [from]="1" and gets the same result. The spec in this PR moved its dates from 2026 to 2030 and removed setInput('from', 1).
Fix: remove this clamp. from already sets the minimum. If the workplace booking forms need a "not in the past" rule, pass it through from.
| @@ -111,6 +114,8 @@ export class DateRangeCalendarComponent implements OnInit, OnChanges { | |||
| public readonly to_date = input<number>(undefined, { alias: 'to' }); | |||
| /** Index of the day to start the week on when displaying the calendar */ | |||
| public readonly offset_weekday = input(0); | |||
| /** Maximum number of days allowed in the selected range (default: 31 days / ~1 month) */ | |||
| public readonly max_range_days = input(31); | |||
There was a problem hiding this comment.
High. All date ranges are now limited to 31 days.
The new input defaults to 31. date-range-field does not pass a value and no caller sets one.
Example: Concierge reports use <date-range-field [from]="0">. Select 1 Jan, then 31 Mar. The calendar disables the days after 31 Jan and clamps the end date, so a quarter or a year cannot be selected. Survey responses and contact tracing use the same field.
Fix: default to 0 for no limit, and set a limit only from the workplace schedule.
| --brand-500: hsl(336, 57%, 48%); | ||
| --brand-600: hsl(336, 57%, 48%); | ||
| --brand-content: hsl(0, 0%, 100%); | ||
| --brand-100: #00afd0; |
There was a problem hiding this comment.
High. The shared stylesheet now has one customer's palette as the default for all apps.
--brand-*, --secondary, --info, --info-light, --warn and the dark --base-* values change in :root and .theme-dark. Each app that loads this file gets them.
Example: light --info-light changes from hsl(207, 89%, 94%) to #00afd0. status-pill and settings-toggle use it as a background colour. libs/components/src/lib/global-loading.component.ts also changes the loading gradient for all apps.
Fix: restore the default values and apply this palette as a theme override for the deployment that owns it. I did not render the result.
| loadComponent: () => | ||
| import('./desk-flow.component').then( | ||
| (m) => m.NewDeskFlowComponent, | ||
| import('./desk-flow-new/desk-flow.component').then( |
There was a problem hiding this comment.
High. The new desk and meeting flows replace the old flows for all deployments, and they do not read many documented settings.
There is no setting to keep the old flow. I searched desk-flow-new/ and meeting-flow-new/ for these keys and found no reads:
- Desk:
desks.allow_groups,desks.can_book_for_others,desks.auto_allocation,desks.has_assets,desks.bookable_hours,desks.needs_reason. - Meeting:
events.booking_unavailable,events.no_standalone,events.can_book_for_others,events.allow_multiday,events.strict_capacity_check.events.hide_attendeesis declared inmeeting-flow-options.component.ts:417, but the template does not use it.
Examples: a tenant sets events.booking_unavailable: true to stop room bookings in the app. After this PR, users can book. A tenant with desks.allow_groups: false now shows the Group and Book for other buttons to all users.
Fix: port these checks into the new steps, or select the flow with a setting until the new flows have parity. The old flow components are now unreachable from the routes. Delete them when parity is done.
There was a problem hiding this comment.
Follow-up on 04c589d6c: partly fixed.
Each setting in this comment is read now and has effect. desks.needs_reason had no reader in the old flow, so there is nothing to port. Its row in docs/settings/workplace.md:268 describes a behaviour that the code does not have.
These documented settings still have no effect in the new flows:
desks.allow_time_changes.desk-flow-details.component.ts:185anddesk-flow-select.component.ts:175show the time and All Day fields with no check. Example: a tenant sets it tofalse, and a user can still book 14:00 to 15:00.desks.allow_all_day.desk-flow-details.component.ts:426uses||withbookings.allow_all_day. The old flow used??, sodesks.allow_all_day: falseno longer has priority.events.can_book_for_anyone. No code inmeeting-flow-new/reads it.events.allow_online_meetings. The online meeting toggle of the shared form is not in the new flow.visitors.reason_required.visitor-flow.component.ts:217uses'Visit'when the reason is empty.bookings.multiple_visitors.visitor-flow-details.component.tsalways shows the Single or Multiple selection. A tenant withfalsecan now send group invites.
Also, events.no_standalone is read at meeting-flow-options.component.ts:599, but meeting-flow.component.ts:174 and meeting-flow-space-select.component.ts:934 require a room before step 3. A meeting with no room cannot be booked in any deployment, and the check and the no-room dialog cannot run.
The author's reply lists desks.allow_time_changes and events.can_book_for_anyone as not done. Fix: port these, or select the flow with a setting until parity is done.
Reviewed by an AI agent: Claude Opus 5.5 in Claude Code, run from T3 Code.
| edit_fn: (b) => this._schedule.editBooking(b), | ||
| remove_fn: async (b, s) => { | ||
| await this._schedule.remove(b, s); | ||
| this.selected_date.set(Date.now()); |
There was a problem hiding this comment.
Low. Cancel on the delete prompt moves the grid to the current week.
ScheduleStateService.remove() returns with no error when the user cancels the prompt. This line then resets selected_date.
Example: view three weeks ahead, open a booking, select delete, then cancel. The grid moves back to this week.
Fix: reload only. Do not set selected_date here.
| @@ -322,7 +323,7 @@ export class Booking { | |||
| } | |||
|
|
|||
| public get is_all_day() { | |||
| return this.all_day || this.duration >= 12 * 60; | |||
| return this.all_day || this.duration >= 18 * 60; | |||
There was a problem hiding this comment.
Low. Booking.is_all_day changes from 12 hours to 18 hours for all apps.
CalendarEvent.is_all_day and docs/STANDARDS.md still use 12 hours.
Example: a 07:00 to 19:00 desk booking showed All Day on booking-card. It now shows a time range.
Optional: if this is for 12 hour parking shifts, make the limit depend on the booking type.
| @@ -949,6 +952,8 @@ export class EventDetailsModalComponent implements OnInit { | |||
| ); | |||
|
|
|||
| public ngOnInit() { | |||
| console.log('Event:', this._data); | |||
There was a problem hiding this comment.
Low. Debug log left in a shared modal.
The PR adds four more: desk-flow-select-list.component.ts:328, auto-assigned-desk-modal.component.ts:457, and schedule-state.service.ts:926 and :979.
Line 956 also builds the CalendarEvent a second time. The field initialiser already does it.
| class="bg-base-100 border-base-300 fixed top-72 right-2 z-10 h-14 w-14 border shadow-md" | ||
| (click)="viewVirtualConcierge()" | ||
| > | ||
| <img class="z-10 h-12" src="assets/icons/roybot.png" /> |
There was a problem hiding this comment.
Low. The virtual concierge button uses a fixed customer image.
roybot.png is 927 KB in shared/assets and renders at 48 px. The image has no alt and the button has no accessible name. ben_icon.svg is added and no code uses it. landing-virtual-concierge.component.ts and chat-view.component.ts are never rendered. They are commented out in landing-new.component.ts.
Optional: take the icon from a setting, make the file smaller, and remove the two unused components and their specs until they are in use.
| @HostListener('touchend', ['$event']) | ||
| public onClick(event: Event) { | ||
| event.stopPropagation(); | ||
| if (typeof this._data.onSelect === 'function') { |
There was a problem hiding this comment.
Low. No code supplies onSelect.
space-map.component.ts passes { ...space, active, selected } as the feature data. So this handler only stops propagation of click and touchend on a pin that now shows a pointer cursor.
Optional: pass onSelect from space-map, or remove the handler.
Keep the branch side of the nine conflicts and apply the type cleanups from develop to it. Type window.gtag as a function and import UserAvatarComponent from the library entry point, so that the merge compiles and passes lint.
The new workplace UIs changed shared code in ways that affected Concierge, the Outlook add-in and other deployments. - Restore the default palette and loading gradient. - Date field: remove the clamp to the current time. Date range calendar: no default range limit. - Analytics: remove the query string from page views and track routes for GA4 IDs only. - Room capacity filter is a minimum again, with an optional capacity_max. - Booking and event forms clear on a visit to the landing page again. - Read the old favourite keys as a fallback when user settings load. - Desk map reads and writes the zones option that the desk filter uses. - Find availability no longer counts desk bookings as busy time. - Booking.is_all_day is 12 hours again, and 18 hours for parking. - Fix the level name in bookingLocationString. - Remove duplicate report keys from ten locale files and add the keys that the new workplace pages use. - Reduce the virtual concierge image from 927 KB to 24 KB.
- Default analytics.tracking_id to an empty string. - Sort bays in natural order, so that B2 comes before B10. - Read the created time of a parking request from the booking. - Add parking.show_user_groups to the settings schema.
…e old ones Settings - Default analytics.tracking_id to an empty string. - Desk flow reads desks.allow_groups, can_book_for_others, auto_allocation, has_assets and bookable_hours. - Meeting flow reads events.booking_unavailable, no_standalone, can_book_for_others, allow_multiday, strict_capacity_check, hide_attendees, allowed_future_days and multiple_spaces. - Landing page reads app.features, feature_groups, hide_colleagues, hide_landing_spaces, hide_landing_rooms, show_quick_links, show_quick_book and the stored hide_nav flag. - Parking requests have no shift presets by default. The new parking.show_special_needs setting replaces the always-on special needs. - The virtual concierge button takes its image from virtual_concierge.icon. Booking flows - A Special Needs parking request can be submitted. - Catering can be added in the meeting flow. - A group visitor invite with no visitors is rejected. - An edit of a group desk booking updates each member. - Book for other with no person is rejected, also from the auto-assign banner. - The auto-assign modal restores the form when it closes with no booking. - Recent visitors from the old form keep their format. Landing, schedule and team schedule - Check-in on the home card uses the instance of a recurring booking. - Available Now has its own availability query. - A stale range end no longer inverts the schedule query. - Bookings for other people show by default. - A multi-day booking shows on each day that it covers. - The day view scrolls to the current time one time for each date. - Team schedule reloads bookings on a week change only.
…th the code Add the virtual concierge, parking waitlist, special needs and future days settings. Update the landing settings for the new page and remove hide_landing_sidebar, which the new layout does not have.
…adient css_variables does not apply in the dark theme, because the dark theme defaults have a higher specificity. The new css_variables_dark setting sets colours for the dark theme only. The loading screen shows before the zone settings load, so it reads its colours from loading_gradient in the domain config. The browser stores the last gradient to prevent a colour change on each load.
Take the Nx 23.3.0 update. The lockfile is the same as on develop.
|
I pushed fixes for the review above. All 38 inline comments and the 6 general findings are addressed, and the branch has the latest Commits
What changedShared libraries and styles
Concierge
Workplace booking flows
Workplace landing, schedule and team schedule
New settings
Settings required for the client deploymentThese values were defaults in the code. They are now removed from the defaults, so the client deployment must have them in its own configuration before it takes this build. Zone metadata {
"analytics": { "tracking_id": "G-S6TDS95BDH" },
"virtual_concierge": { "icon": "assets/icons/roybot.png" },
"css_variables": {
"brand-100": "#00afd0",
"brand-200": "#006a9d",
"brand-300": "oklch(0.95 0.004 236.498)",
"brand-400": "#5ac28d",
"brand-500": "#00917b",
"brand-600": "#e77d9a",
"secondary": "#006a9d",
"secondary-focus": "#00afd0",
"info": "#006a9d",
"info-light": "#00afd0",
"warn": "hsla(45, 100%, 64%, 1)"
},
"css_variables_dark": {
"brand-100": "#00afd0",
"brand-200": "#006a9d",
"brand-300": "oklch(0.95 0.004 236.498)",
"brand-400": "#5ac28d",
"brand-500": "#00917b",
"brand-600": "#e77d9a",
"secondary": "#006a9d",
"secondary-focus": "#f68c50",
"base-100": "oklch(23% 0.034 264.665)",
"base-200": "oklch(27.9% 0.041 260.031)",
"base-300": "oklch(37.2% 0.044 257.287)",
"base-400": "oklch(44.6% 0.043 257.281)",
"info": "#00afd0",
"info-light": "hsl(190, 100%, 50%)"
},
"parking": {
"show_special_needs": true,
"request_shift_options": [
{ "id": "day_worker", "name": "BOOKINGS.PARKING_SHIFT_DAY_WORKER", "start_time": 420, "end_time": 1020 },
{ "id": "day_shift_12hr", "name": "BOOKINGS.PARKING_SHIFT_DAY_12HR", "start_time": 330, "end_time": 1110 },
{ "id": "night_shift_12hr", "name": "BOOKINGS.PARKING_SHIFT_NIGHT_12HR", "start_time": 1050, "end_time": 390 },
{ "id": "half_day_am", "name": "BOOKINGS.PARKING_SHIFT_HALF_DAY_AM", "start_time": 420, "end_time": 720 },
{ "id": "half_day_pm", "name": "BOOKINGS.PARKING_SHIFT_HALF_DAY_PM", "start_time": 750, "end_time": 1020 }
],
"request_submission_notes_html": "<p>Allocation of parking spaces for the following week will occur the <strong>Friday afternoon prior</strong>. Requests received after allocation has occurred will automatically be <strong>waitlisted</strong> until a suitable space becomes available.</p><p>Approval depends on your <strong>priority group</strong>.</p><p>After hours requests require manual approval.</p>"
}
}Zone metadata {
"analytics": { "tracking_id": "G-S6TDS95BDH" }
}Add the same Domain config, in the {
"loading_gradient": ["#f15b55", "#f68c50"]
}Two client items have no setting and return to the shared text.
Behaviour changes for other deployments
Not done
VerificationOn the final tree, with Nx 23.3.0 and the same Bun version as CI:
I loaded the landing page, the schedule page and the new desk flow in mock mode, in the light theme and the dark theme. I did not submit a booking in a browser. Specs cover the submit paths. Mock mode has no domain config, so unit tests cover the loading gradient. An independent review of the fixes ran before the push. It confirmed two defects in the new desk flow, and both are fixed with specs. A group booking was saved as a single booking after a return to the form, and Book for other with no person was saved for the current user through the auto-assign banner. Posted by an AI agent: Claude Opus 5.5 in Claude Code, run from T3 Code. |
Conflicts in the schedule service imports and its spec, and in the bookings spec imports. Both sides are kept. The schedule specs for the range end date and the deleted flag now read the paged query stub.
MrYuion
left a comment
There was a problem hiding this comment.
Follow-up review of 597bdd67e..04c589d6c. I reviewed the five fix commits one by one, and the conflict resolutions of the three develop merges in that range (6b7dadc90, d957502f9, 04c589d6c). I read the code. I did not run the apps or the tests. CI builds are green on this head.
Status of the earlier findings
Inline comments: 37 of 38 are fixed. I checked each one in the source at 04c589d6c.
Partly fixed:
apps/workplace/src/app/book/book.routes.ts:44. The new flows now read each setting that the comment listed. Six more documented settings still have no effect. My reply in that thread has the list.
General findings: 3 of 6 are fixed.
- Settings docs. Fixed. Two small mismatches are new. See the inline comment on
docs/settings/workplace.md:499. - Tests for the submit paths. Fixed.
confirmBookingin the desk and visitor flows, the Special Needs path and the home card check-in have specs now. - Dead code. Not fixed, as the author's reply states. The old landing and flow components,
schedule-mobile-calendar.component.ts,landing-virtual-concierge.component.ts,chat-view.component.tsandben_icon.svgare still in the tree. - Type safety. Partly fixed. The diff added about 90 lines with
anyat the last review. It adds 26 now, in 7 files. 16 of them are$any()calls inschedule-day-view.component.tsandschedule-week-view.component.ts. - Duplicate locale keys. Fixed. A script found no duplicate key in the locale files.
- Hard-coded English. Partly fixed. Examples that remain are
visitor-flow-recent.component.ts:22and:26, andparking-request-form-details.component.ts:2178.
Merge resolutions. Correct in all three merges. No PR change and no develop change was lost. schedule-state.service.ts has the page loop from #562 and the range period from this PR.
New findings
4 medium and 7 low. Findings 2 to 11 have an inline comment.
Medium
- The branch conflicts with
developagain, anddevelopnow has an e2e suite for the old pages.developis 55 commits ahead of this head. A test merge has one conflict, inapps/workplace/src/app/app.component.ts, and GitHub shows the PR as conflicting. The larger problem isapps/workplace/e2e/local/from #497, 74 files inapps/workplace/e2e. Those specs find the old components by selector. Examples:bookings-edit.spec.ts:79expectsdesk-flow-form,bookings-edit.spec.ts:138expectsinvite-visitor-form, andhome-meeting-with.spec.ts:63expectsmeeting-flow-form. The routes in this PR do not render these components.e2e-advisory.ymlruns on each push todevelopand does not block a build, so the failures will show after the merge, not on this PR. Fix: mergedevelop, then update the e2e suite for the new pages or run it one time against this branch before the merge. I did not run the suite. - Meeting confirm does not check that the catering orders are still available.
meeting-flow-options.component.ts:569. - Favourite parking spaces and lockers do not show on the home card.
landing-favourites-new.component.ts:219. - A multi-day all-day booking reads as ended after its first day.
schedule-state.service.ts:1113.
Low
- A hidden range end can come back and give a range that the user did not select.
schedule-state.service.ts:213. - The international flag of a recent visitor is lost.
visitor-flow-invites.component.ts:319. - With
desks.auto_allocation, a second click on Confirm can save two bookings.desk-flow.component.ts:416. - Available Now can read the desk settings before the building settings load.
landing-available-now.component.ts:238. - Favourite rooms show for the active building only.
landing-favourites-new.component.ts:203. - Two rows of the virtual concierge docs do not match the code.
docs/settings/workplace.md:499. - The new scroll spec passes without the fix.
schedule-day-view.component.spec.ts:317.
Found in older PR code
These lines are not from the fix commits, and the first review did not report them. I found them while I checked the fixes. All paths are in apps/workplace/src/app/.
book/meeting-flow-new/meeting-flow.component.ts:203. The step number changes only when the URL has aviewparameter. Open the form, select Search Rooms, then press the browser Back button. The URL has noviewand the page stays on step 2. The router also keeps the component after a booking, so Meeting in the top menu opens step 3 with an empty form. Fix: setviewto 0 when the parameter is absent.schedule/schedule-list-view.component.ts:110.if (this.loading()) return [];makes the list empty on each poll. With a 31 day list, the cards are destroyed and created again each 3 minutes and the scroll position is lost. Fix: remove the line. The page already shows a progress bar.landing-new/landing-favourites-new.component.ts:210to:230. The threeresource()loaders have nocatch.resource.value()throws when the resource is in the error state, so one failed metadata request makes thefavourites()computed throw in the template. The other resources on the page catch errors. Fix: add.catch(() => [])to each loader.book/meeting-flow-new/meeting-flow-options.component.ts:333.asset-list-fieldgets noresources, the same defect that the catering field had. A room in the assetdisabled_roomslist does not disable the field. Fix: addresources: form_value().resources.
One item is from develop, not from this PR. queryAllBookings in libs/bookings/src/lib/bookings.fn.ts:272 ends with unique(list, 'id'). Occurrences of a recurring booking have one id and different instance values, so the week view and the new list range can show only the first occurrence. #562 moved the schedule to this function. I confirmed the code path. I did not check the API response.
Suggested improvements
- Fix
date_endinBookingandCalendarEventfor all-day items of two or more days. ThenitemEndinschedule-state.service.ts:104can go, and finding 4 is fixed for all callers. - The date, time, duration and multi-day fields are in three templates of the meeting flow:
meeting-flow-details.component.ts:59andmeeting-flow-space-select.component.ts:191and:542. One small component can replace them. openAddColleaguesModalhas three copies, in the team schedule table, the team schedule list andlanding-colleagues-new.component.ts.landingFeatures()inlanding-features.ts:11repeats the feature group check oftop-menu.component.ts:270.- In
parking-request-form.component.ts:270,_user_special_needsis set one time fromuser_details(). Acomputedoveruser_details()is shorter, and it also covers a parking user list that loads after the form. schedule.component.ts:165casts theviewquery parameter toScheduleView. Accept only the three known values.setDateRangeinschedule-state.service.ts:499andfindColleagues()inlanding-upcoming-booking.component.ts:678have no caller in app code.- Delete the old flow and landing components when parity is done.
desk-form-details.component.tsmust stay, because Concierge imports it.
Checked and found correct
- The default palette and the loading gradient are the same as
develop.css_variables_darkusesbody.theme-dark, which has a higher specificity than.theme-dark. loadingGradientaccepts only colour values, and a value such asurl(...)gives the default gradient.- The date field has no clamp to the current time. The date range calendar has no default limit, and the two schedule calendars pass
[max_range_days]="31". - GA4 page paths have no query string or fragment. A GTM container gets one page view. Both apps default to an empty tracking ID.
- The capacity filter is a minimum, and only the new meeting flow sends
capacity_max. - The two form services clear on a visit to
/landing. The landing widgets fill the form after the navigation, or pass a query parameter. - The old favourite keys are read as a fallback, and the next save writes the new keys.
- The desk map and the desk filter both use
zones. - Find availability is the same as
develop, andlibs/usersno longer importslibs/bookings. - The concierge bay sort and the request Created column.
Not verified
- Runtime behaviour. Findings 7, 8 and 11 depend on timing, and I confirmed them from the code only.
- The test suites. I relied on the author's report for the unit tests.
- The other locales. The new keys are in
en-AU.jsononly.
Reviewed by an AI agent: Claude Opus 5.5 in Claude Code, run from T3 Code.
| setTimeout(() => this._input?.nativeElement?.focus(), 100); | ||
| } | ||
|
|
||
| public async confirmBooking() { |
There was a problem hiding this comment.
Medium. Confirm does not check that the catering orders are still available.
The old flow called _cateringOrdersAvailable() before the save and showed CATERING_UNAVAILABLE (meeting-flow/meeting-flow-form.component.ts:685). This commit ported the other checks of viewConfirm and made catering usable in step 3, but this check is not here.
Example: add a catering order in step 3. Go back to step 1 and move the meeting to a time that the menu does not serve. Return to step 3 and confirm. The meeting saves with the order.
A room change has a similar risk: an order can keep the system_id of a room that is no longer selected. I did not confirm that case.
Fix: port _cateringOrdersAvailable() into confirmBooking, after the form validity check. Add a spec with an order that orderAvailable rejects.
| ); | ||
| private readonly _parking_resource = resource({ | ||
| params: () => this._buildingWithFeature('parking'), | ||
| loader: () => this._booking_form.loadResourceList('parking-spaces'), |
There was a problem hiding this comment.
Medium. Favourite parking spaces and lockers do not show on the home card.
loadResourceList('parking-spaces') reads level metadata with that name, through loadMetadataResources. The parking flow reads its spaces from the assets API (loadParkingResources, then queryParkingSpacesForZones). No other code reads parking-spaces metadata. Line 226 has the same problem for lockers, which the booking flow loads from the locker banks.
Example: a user adds bay P1 to the favourites in the parking flow. favourite_parking holds the asset ID. This list is empty, so the card shows no parking favourite. The old page used loadParkingResources() (landing/landing-favourites.component.ts:357).
The new spec asserts the parking-spaces key, so it passes with the defect.
Fix: use this._booking_form.loadParkingResources() and loadLockerResources(this._org, building_id), the same as landing-available-now.component.ts:285. Lines 261 and 289 then need [item.zone?.id], because these items have zone and not zones.
| const resource_name = item.asset_name || item.asset_id; | ||
|
|
||
| // Fail if the current time is past the booking end | ||
| if (isAfter(now, item.date_end)) { |
There was a problem hiding this comment.
Medium. A multi-day all-day booking reads as ended after its first day.
overlapsDay and itemEnd put the booking on each day that it covers. The Booking class still sets date_end to the end of the first day (snapToWholeDays, libs/common/src/lib/types/booking.class.ts:179). has_ended, the ended status and this check all use date_end.
Example: a Monday to Wednesday all-day desk booking, viewed on Tuesday. End gives the "already ended" error. The booking has the ended status, and the time text at schedule-day-view.component.ts:180 shows the end of Monday.
Before this fix the booking did not show on Tuesday, so the user could not get to this state.
Fix: set date_end to the start plus the duration in snapToWholeDays, and the same in event.class.ts. itemEnd is then not necessary. If that change is too wide for this PR, use itemEnd(item) here and in the two templates.
| public readonly end_date = computed(() => { | ||
| const end = this._end_date(); | ||
| if (end === null) return null; | ||
| return end >= startOfDay(this._date()).valueOf() ? end : null; |
There was a problem hiding this comment.
Low. A hidden range end can come back.
This guard hides _end_date when it is before the date, but no code clears it. setDate() at line 491 changes _date only, and the change of view keeps the old end.
Example: in the list view select 5 to 9 Oct. Change to the day view and select 1 Sep. Change back to the list view. The range is 1 Sep to 9 Oct. That is 39 days, the user did not select it, and it is more than the 31 day limit of the calendar.
Fix: clear _end_date in setOptions when the period changes to a value other than range.
| company: item.company || '', | ||
| }; | ||
|
|
||
| this.model.update((m) => ({ |
There was a problem hiding this comment.
Low. The international flag of a recent visitor is lost.
parseRecentVisitors reads the flag, but this update does not copy it into the model. _saveRecentVisitors in visitor-flow.component.ts then writes the model value back to the history.
Example: the shared invite form saved a@x.com|A|Acme|1. Select this visitor here and confirm. The booking has international: false, and the history item becomes a@x.com|A|Acme|0|. The shared form then reads the visitor as not international.
visitor-flow-recent.component.ts:232 has the same update, and the group branch of _saveRecentVisitors passes no flag.
Fix: copy international: !!item.international in the two updates, and pass it for group members.
| if (this.other_user_error()) { | ||
| return notifyError(this.other_user_error()); | ||
| } | ||
| if (this.auto_allocation) { |
There was a problem hiding this comment.
Low. With desks.auto_allocation, a second click on Confirm can save two bookings.
loading becomes true at line 447, after await autoAllocateDesk(). The button is disabled by loading() only.
Example: the user clicks Confirm two times while the desk list loads. The two calls each get a desk and continue to _postBooking(). The old flow opened a confirm sheet at this point, so a second click did not save.
I confirmed this from the code. I did not run it.
Fix: return at the start of the method when loading() is true, set it to true there, and put the full method in one try and finally.
| private readonly _available = resource({ | ||
| params: () => { | ||
| const type = this.tab_details[this.active_tab()]?.type; | ||
| const building_id = this._org.active_building()?.id; |
There was a problem hiding this comment.
Low. Available Now can read the desk settings before the building settings load.
The loader runs when active_building() has an ID. loadResourceList('desks') then reads desks.use_assets and use_region one time. BookingFormService._requests_ready waits for active_building_loaded() and the override count, and its comment says that building and region settings read as their defaults before that time (libs/bookings/src/lib/booking-form.service.ts:354).
Example: desks.use_assets: true is in the building metadata only. On a first load of /landing, the loader reads level metadata, finds no desks and shows the empty message. It does not load again until the tab or the building changes.
I confirmed this from the code. It depends on load order and I did not run it. The _desks_resource of the favourites card has the same condition.
Fix: return undefined from params until this._org.active_building_loaded() is true.
| * is from the landing state: the event form loads its space list only | ||
| * after the user opens a booking page. | ||
| */ | ||
| public readonly all_spaces = computed(() => |
There was a problem hiding this comment.
Low. Favourite rooms show for the active building only.
The list is now LandingStateService.space_list(), which has the rooms of the active building. The old page resolved each favourite ID with SpacePipe (landing/landing-favourites.component.ts:347), so the building did not matter.
Example: the active building is A and the user has a favourite room in building B. The card does not show the room.
Fix: resolve the favourite IDs with SpacePipe, as the old page did.
| | `virtual_concierge.icon` | string | – | URL of the image for the button. A default icon shows when this is empty. | | ||
| | `virtual_concierge.display` | string | `"fullscreen"` | How the embed opens: `fullscreen` or `sidebar`. | | ||
| | `virtual_concierge.side` | string | `"left"` | Side of the screen for the sidebar: `left` or `right`. | | ||
| | `virtual_concierge.prompts` | string[] | three example prompts | Suggested prompts for the virtual concierge UI. | |
There was a problem hiding this comment.
Low. Two parts of this section do not match the code.
virtual_concierge.promptshas one reader,landing-virtual-concierge.component.ts:215. That component is commented out inlanding-new.component.ts:32, so the setting has no effect.- Line 490 lists five pages. The button is also on the landing, book, group events and control pages.
Fix: remove the prompts row, its default and its schema property until the component is in use. Correct the page list, or say that the button is on each main page.
| }); | ||
| }); | ||
|
|
||
| it('should scroll to the current time one time for each date change', () => { |
There was a problem hiding this comment.
Low. This spec passes without the fix.
beforeEach creates the component with real timers, so ngOnInit registers the 10 second interval with the real setInterval. vi.useFakeTimers() runs after that, and advanceTimersByTime(30 * 1000) does not fire the interval. _changed does not change, so the old effect also scrolls one time in this test.
I read the spec. I did not run it against the old code.
Fix: call vi.useFakeTimers() before createComponent() in this test. Also restore Element.prototype.scrollIntoView at the end.
Old views still exist if they are needed.