Skip to content

fix(bookings): keep form input across the reset flows actually run (PPT-2643) - #479

Open
camreeves wants to merge 2 commits into
developfrom
fix/PPT-2643-remaining-init-paths
Open

fix(bookings): keep form input across the reset flows actually run (PPT-2643)#479
camreeves wants to merge 2 commits into
developfrom
fix/PPT-2643-remaining-init-paths

Conversation

@camreeves

Copy link
Copy Markdown
Contributor

Reopens and finishes PPT-2643. #478 fixed one path; it is not the path the booking flows take.

Why the first fix does not fire

newForm protects its currentUserIsLoaded() deferral branch. In the running app that branch is not reached: the current user is restored from the localStorage cache about 50ms after bootstrap (libs/common/src/lib/user-state.ts:347, :217-223), while every flow calls its form lifecycle only after org data lands — NewDeskFlowComponent.ngOnInit awaits waitUntilInitialised() plus a hard 300ms settle, then calls loadForm('desk') and, for a fresh booking, newForm('desk') back to back (apps/workplace/src/app/book/desk-flow.component.ts:56-66). By then currentUserIsLoaded() is true, so the deferral never fires and _pending_user_edits is null when the replay would happen.

Meanwhile the form has been interactive since first paint — desk-form-details gates only on form() && model, both truthy from field initialisation — so a user can type into it for the whole of that window.

loadForm is the first of the two resets and had no capture at all. Its model.set({ ...bookingFormValue(new Booking()) ... }); this.form().reset(); restores defaults over the user's input: all_day back to false, and a truthy secondary_resource default that re-checks "Require locker" — which, with no lockers available, silently invalidates the form.

The fix

Capture in loadForm as well, and replay over the loaded booking before applyDurationSettings so a restored all_day still drives the time-sync window — the same ordering #478 used.

Two details worth reviewing:

  • The capture merges rather than replaces. form().reset() clears the dirty flags _userEditedValues() reads, so by the time the chained newForm captures there is nothing left to find; a plain assignment would replace a real capture with an empty one.
  • The stash is released on a microtask — late enough for that chained newForm in the same tick, early enough that it cannot leak into an unrelated form opened later. The second spec pins that.

Verification

Two new specs in libs/bookings/src/test/booking-form.service.spec.ts, both seen red before the change (expected 'Booking' to be 'Quiet corner desk'). They drive the ordinary path — no mocking, and unlike the #478 specs they do not neutralise the test-runtime probe, because this path does not need the deferral.

nx test bookings 37/37, nx test workplace 68/68, nx build workplace clean.

What is not covered

The e2e suite still cannot detect this, and I would not trust it to. The race needs initialisation to be slow relative to typing; on a fast machine org init finishes before Playwright can reach the field, so the desk-booking flow passes with or without the fix. The converging workaround in bookDeskViaUI therefore stays for now, and REG-10 in E2E_USER_STORIES.md stays blocked — see #476 for the measurements, including two attempts that produced red runs for reasons that turned out to be the test's fault rather than the app's.

🤖 Generated with Claude Code

PPT-2643 again. #478 fixed `newForm`'s deferred re-entry, but that branch is
not the one the booking flows take. The current user is restored from the
localStorage cache within about 50ms of bootstrap, whereas every flow calls
its form lifecycle only after org data lands — `NewDeskFlowComponent.ngOnInit`
awaits `waitUntilInitialised()` plus a 300ms settle, then calls `loadForm`
and, for a fresh booking, `newForm`. So `currentUserIsLoaded()` is already
true, the deferral never fires, and the captured-edits replay never runs.

`loadForm` had no capture at all, and it is the first of the two resets. Its
`model.set(...)` restores defaults — `all_day` false, a truthy
`secondary_resource` that re-checks "Require locker" — over whatever the user
typed into a form that has been interactive since first paint.

Capture in `loadForm` too, and replay over the loaded booking before
`applyDurationSettings` so a restored `all_day` still drives the time-sync
window. The capture merges rather than replaces, because `form().reset()`
clears the dirty flags the capture reads, so the `newForm` that follows in the
same tick would otherwise overwrite a real capture with an empty one. The
stash is released on a microtask, which is late enough for that chained reset
and early enough that it cannot reach an unrelated form.

Two specs, both seen red first, driving the ordinary path with no mocking and
no runtime probe neutralised: input entered before initialisation survives
`loadForm` + `newForm`, and it is not resurrected in a later form.

Fixes PPT-2643

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@vercel

vercel Bot commented Aug 5, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
frontend-templates Ignored Ignored Preview Aug 5, 2026 10:41am

camreeves added a commit that referenced this pull request Aug 5, 2026
The previous note said there was no evidence the fix was incomplete. There
is; it just was not going to come from this suite. `newForm`'s protected
branch is never taken by the flows, because the current user is restored from
cache long before org data lands, and `loadForm` — which the flows call first
— had no capture at all.

The suite passing 8/8 either way is the finding, not a reassurance: REG-10
stays blocked once #479 lands, because this hardware cannot lose the race.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@camreeves

Copy link
Copy Markdown
Contributor Author

On the red test (concierge) check — it is pre-existing and timezone-dependent, not from this change.

The two failures are parking-bookings-list.component.spec.ts > should show all day when the booking matches the bookable period and parking-map.component.spec.ts > should select a one-hour parking availability window.

Controlled comparison, since "it passed locally" was not good enough — my machine is Australia/Sydney and CI runs UTC:

Sydney TZ=Etc/UTC
develop's booking-form.service.ts (no change) 246/246 pass same 2 fail
this branch 246/246 pass same 2 fail, none extra

So the check goes red on this branch for the same reason it would on any branch cut today. develop itself currently has a red run on the sibling test explore-parking.service.spec.ts > should clamp all-day parking queries to bookable hours — same family: all-day windows clamped against bookable hours, evaluated against the real clock.

nx test bookings is 37/37 under UTC as well, including the two new specs here.

Worth someone pinning those parking specs to a fixed clock — they will keep going red for whoever is unlucky with the hour. Happy to raise it separately; it is not this PR's to fix.

Switching between booking forms without leaving the booking area does not
reset the form, so edits captured for the initialisation replay follow the
user across. That is a consequence of the fix worth stating rather than
discovering later: only fields the user actually edited move, isCrossTypeEdit
still discards the previous booking's identity, and leaving the section
calls clearForm().

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant