Skip to content

fix(bookings): keep user input entered while the booking form initialises - #478

Merged
MrYuion merged 1 commit into
developfrom
fix/PPT-2643-preserve-booking-form-input
Aug 4, 2026
Merged

fix(bookings): keep user input entered while the booking form initialises#478
MrYuion merged 1 commit into
developfrom
fix/PPT-2643-preserve-booking-form-input

Conversation

@camreeves

@camreeves camreeves commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Fixes PPT-2643.

The bug

BookingFormService.newForm() defers itself until the current user has loaded:

if (!currentUserIsLoaded()) {
    currentUserLoaded().then(() => this.newForm(type, booking));
    return;   // <- returns; the form is already rendered and interactive
}
...
this.model.set(bookingFormValue(new Booking()));   // <- wipes
this.form().reset();                               // <- wipes

The form accepts input during that window. When initialisation completes it resets, silently discarding whatever was entered: title reverts to the default Booking, All Day switches back off, Require locker switches back on.

On a fast connection nobody notices because init finishes first. On a slow one you start typing a title and it vanishes a second later. It affects every flow through this shared service, so desk, locker, parking, visitor and room, across workplace and the other booking UIs.

The fix

Option A from the ticket: preserve the input rather than gate interactivity.

The user's edits are captured on the way back into newForm and replayed over the incoming booking. Only fields the user actually touched are carried, read from the signal-forms dirty flags, so programmatic writes (_patch, model.set) are ignored and a field the user never touched still takes its value from the booking being opened.

Placement matters and is commented in the source. The replay happens after the main patch but before applyDurationSettings(), so a restored all_day still drives the time-sync window, and before _syncWindowIfUnchanged(), which compares against the initial window and so yields to a user-changed one without any extra handling.

The capture is set synchronously immediately before the re-entry, so there is no window in which a normal newForm call could consume edits meant for a deferred one.

I did not take Option B (gating interactivity behind a loader) because it is a visible UX change across every booking flow and is yours to call.

Testing

Three specs in booking-form.service.spec.ts under initialisation while the user is still loading.

No mocking. currentUserIsLoaded() reports "loaded" whenever it detects a test runtime, so the tests neutralise that runtime probe to reach the deferred branch, which keeps the real promise plumbing in currentUserLoaded() under test rather than a stub of it.

Red-checked against unfixed code, where two of the three fail with exactly the ticket's symptoms:

AssertionError: expected 'Booking' to be 'Quiet corner desk'
AssertionError: expected false to be true

The third (does not resurrect that input on the next new form) passes either way by design. It guards against the new code leaking preserved edits into a later form rather than reproducing the original bug.

  • nx test bookings — 393 passed
  • nx affected -t test — 25 projects, all passed
  • nx affected -t build — 16 projects, all built

nx lint fails in this workspace on untouched libraries too (Failed to load config "plugin:@angular-eslint/recommended"), so it is pre-existing and unrelated.

Follow-up

The workplace e2e suite currently works around this bug in bookDeskViaUI with a converging retry block, which means it no longer detects it. Once this merges that workaround should come out and be replaced with a spec asserting input survives initialisation. That lives on the e2e branches (#476/#477) and is not touched here.

How it was found: the new e2e suite, which produced bookings saved under the wrong title locally and an unopenable confirm dialog in CI (a reverted All Day leaves the default 5-minute slot, which on a slow run has already passed and invalidates the form).

🤖 Generated with Claude Code

…ises

`newForm` defers itself until the current user has loaded, then re-enters
and resets the form. The form is already rendered and interactive by then,
so anything typed or toggled in that window was silently destroyed: the
title reverted to the default, All Day switched back off, Require locker
switched back on.

It only bites on a slow load, which is why it reads as the app being
unreliable rather than as a reproducible bug.

Carry the user's edits across the deferred re-entry and replay them over
the incoming booking. Only fields the user actually touched are carried,
read from the signal-forms dirty flags, so programmatic writes are ignored
and a field left alone still takes its value from the booking being opened.

Replayed before `applyDurationSettings`, so a restored `all_day` still
drives the time-sync window, and before `_syncWindowIfUnchanged`, which
compares against the initial window and so leaves a user-changed one alone
of its own accord.

Fixes PPT-2643

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@vercel

vercel Bot commented Aug 4, 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 Aug 4, 2026 3:54am

@camreeves
camreeves requested review from MrYuion and stakach August 4, 2026 03:54
@MrYuion

MrYuion commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

LGTM

@MrYuion
MrYuion merged commit a036048 into develop Aug 4, 2026
34 of 35 checks passed
@MrYuion
MrYuion deleted the fix/PPT-2643-preserve-booking-form-input branch August 4, 2026 04:06
camreeves added a commit that referenced this pull request Aug 5, 2026
…ject, boot assertions

Merges develop to pick up #478 (the PPT-2643 fix) and applies the review
feedback on #476.

CI hardening. The job now declares `permissions: contents: read` and checks
out without persisting credentials: the repository default is `write`, and
this is the only job on a self-hosted machine, where a token written to disk
outlives the run. The failure notification drops
`fjogeleit/http-request-action@master` for plain curl — same request, but a
mutable ref should not execute on a box inside the internal network. The six
other call sites are the same pattern on GitHub-hosted runners and are left
for a repo-wide change.

Preflight becomes a setup project that only `local` depends on, instead of a
`globalSetup` that ran for every invocation. `--project=mock` is documented
as needing no backend and now genuinely does not — verified by running it
with the stack down. A missing stack also reports as a named failing test
rather than a runner crash.

The boot smoke test asserts on unexpected server errors instead of only
logging them; a required endpoint could previously start failing while the
test stayed green. The tolerated set is derived from an observed CI run
rather than from documentation, scoped to 5xx and to same-origin API paths.

Records the backend inputs (resolved image IDs, www-core HEAD) in the job
summary so a changed nightly is attributable, and pins keydb by multi-arch
digest. The PlaceOS services stay on ${PLACEOS_TAG} deliberately: catching
backend regressions is what an advisory nightly is for, and PLACEOS_TAG is
already the knob for pinning when bisecting.

Documents that mock is Chromium-only on purpose, and corrects the CI section
of the README, which described a `pull_request` trigger that deliberately
does not exist.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
camreeves added a commit that referenced this pull request Aug 5, 2026
An earlier note claimed PPT-2643's fix did not cover the desk-booking route,
on the strength of the suite going red when the converging block in
bookDeskViaUI was deleted. That was wrong twice over, and both errors are
recorded so the next person does not repeat them.

The first red was a Playwright strict-mode violation rather than a reverted
value: opening the desk-select modal puts a second "All Day" checkbox in the
DOM, bound to the same form field, so an unscoped locator matched two
elements and threw — with both of them checked. Scoping the locator to
desk-flow-form then broke it a second way, because setCheckbox returns
silently when its locator matches nothing, so a narrower scope became a
no-op and the form really was invalid.

The shipped helper is unchanged and stable: six consecutive full runs, 8/8,
at --retries=0. Whether the fix is complete remains unproven in either
direction — the race needs a slower machine than this one — so the block
stays and REG-10 stays blocked, now with the two dead ends written down and
a note that settling it needs throttled CPU or a unit test, not another
e2e attempt.

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.

2 participants