Wait for Blazor to boot before driving the E2E sign-in flow #210

rob · 2026-08-21T11:01:22Z

rob commented

2026-08-21 11:01:22 +00:00

Fixes the flaky E2E suite that has main red.

The defect. PlaceMark.WebUI is pure Blazor WebAssembly with no prerendering, so every top-level navigation — GotoAsync, ReloadAsync, or an external redirect chain (OIDC) landing back on the WebUI — reboots the whole runtime in the browser. JourneySteps.RegisterAndSignInAsync did two full page.GotoAsync loads and started filling fields and clicking the moment Playwright's own actionability checks passed. Nothing anywhere in the suite waited for the app to become interactive.

Run #880 on an idle runner failed 4 of 59, all inside that shared step, with System.TimeoutException: Timeout 30000ms exceeded. A different set of tests failed on each run — the signature of flaky shared setup, not a broken journey.

The fix. App.razor.cs sets <html data-app-ready="true"> from OnAfterRenderAsync(firstRender: true) via appReady.js — a signal the app actually exposes, not a sleep. JourneySteps.WaitForAppReadyAsync waits on it after every navigation in the suite that reboots the runtime: GotoAsync, every page.ReloadAsync() (now routed through a JourneySteps.ReloadAsync wrapper so a future reload can't omit it), and the OIDC sign-in redirect chain's own landing back on the WebUI — a reboot driven by neither Playwright method, found in review and now covered at the one point control returns. RegisterAndSignInAsync's second GotoAsync is removed outright: /login is now reached by clicking Register.razor's own "Sign in" link, a client-side navigation, so a journey boots the runtime once instead of twice.

Every wait is watched to fail before being trusted: CDP Emulation.setCPUThrottlingRate reproduces the exact TimeoutException from Run #880 on an idle box, and moves the failure inside the wait once it's added — proof it genuinely blocks. The OIDC-landing wait needed the same proof independently, under the same throttling technique, once a first attempt (delaying App's own OnAfterRenderAsync directly) turned out not to gate at all — Blazor's rendering is concurrent, so delaying one component's after-render hook doesn't delay a sibling page's own render.

Task 273 (el.placeMarkMap read before createMap's interop call assigns it) is closed for every place in the suite that reads it: RegisterAndSignInAsync waits for it directly, and every ReloadAsync site does too, after a full audit found three reload sites without the wait review's first pass missed.

Boot-path enumeration. ADR-0173 names every mechanism this app's runtime can reboot through today — GotoAsync, ReloadAsync, an external-redirect return (OIDC), and a fourth, NavigationManager.NavigateTo(..., forceLoad: true) on an auth-expiry redirect, which exists in production code but is exercised by no E2E test today — and says plainly that this is not a standing guarantee, not a fourth claim of completeness.

RateLimiting__Auth__PermitLimit goes 100 to 200, test-fixture only (PlaceMarkAppFixture.cs, no production appsettings*.json touched). It's a consequence of the fix, not a workaround: a faster suite concentrates its register/sign-in traffic into less wall-clock time, more often landing inside one FixedWindowRateLimiter window.

Verified on the rebased tree, pinned SDK 10.0.100: E2E 60/60 twice (~1.85 min each, confirmed against this branch's own freshly spawned subprocesses and Postgres container, not any already-running instance), Architecture 148/148, WebUI 916/920 (4 pre-existing skips), Api 622/622, Infrastructure 360/360, Domain 39/39, Contracts 139/139. Coverage ratchet (Cobertura interface) — WebUI holds at 91.6%, no assembly below baseline.

Fixes the flaky E2E suite that has `main` red. **The defect.** `PlaceMark.WebUI` is pure Blazor WebAssembly with no prerendering, so every top-level navigation — `GotoAsync`, `ReloadAsync`, or an external redirect chain (OIDC) landing back on the WebUI — reboots the whole runtime in the browser. `JourneySteps.RegisterAndSignInAsync` did two full `page.GotoAsync` loads and started filling fields and clicking the moment Playwright's own actionability checks passed. Nothing anywhere in the suite waited for the app to become interactive. Run #880 on an idle runner failed 4 of 59, all inside that shared step, with `System.TimeoutException: Timeout 30000ms exceeded`. A different set of tests failed on each run — the signature of flaky shared setup, not a broken journey. **The fix.** `App.razor.cs` sets `<html data-app-ready="true">` from `OnAfterRenderAsync(firstRender: true)` via `appReady.js` — a signal the app actually exposes, not a sleep. `JourneySteps.WaitForAppReadyAsync` waits on it after every navigation in the suite that reboots the runtime: `GotoAsync`, every `page.ReloadAsync()` (now routed through a `JourneySteps.ReloadAsync` wrapper so a future reload can't omit it), and the OIDC sign-in redirect chain's own landing back on the WebUI — a reboot driven by neither Playwright method, found in review and now covered at the one point control returns. `RegisterAndSignInAsync`'s second `GotoAsync` is removed outright: `/login` is now reached by clicking `Register.razor`'s own "Sign in" link, a client-side navigation, so a journey boots the runtime once instead of twice. Every wait is watched to fail before being trusted: CDP `Emulation.setCPUThrottlingRate` reproduces the exact `TimeoutException` from Run #880 on an idle box, and moves the failure *inside* the wait once it's added — proof it genuinely blocks. The OIDC-landing wait needed the same proof independently, under the same throttling technique, once a first attempt (delaying `App`'s own `OnAfterRenderAsync` directly) turned out not to gate at all — Blazor's rendering is concurrent, so delaying one component's after-render hook doesn't delay a sibling page's own render. **Task 273** (`el.placeMarkMap` read before `createMap`'s interop call assigns it) is closed for every place in the suite that reads it: `RegisterAndSignInAsync` waits for it directly, and every `ReloadAsync` site does too, after a full audit found three reload sites without the wait review's first pass missed. **Boot-path enumeration.** ADR-0173 names every mechanism this app's runtime can reboot through today — `GotoAsync`, `ReloadAsync`, an external-redirect return (OIDC), and a fourth, `NavigationManager.NavigateTo(..., forceLoad: true)` on an auth-expiry redirect, which exists in production code but is exercised by no E2E test today — and says plainly that this is not a standing guarantee, not a fourth claim of completeness. **`RateLimiting__Auth__PermitLimit` goes 100 to 200, test-fixture only** (`PlaceMarkAppFixture.cs`, no production `appsettings*.json` touched). It's a consequence of the fix, not a workaround: a faster suite concentrates its register/sign-in traffic into less wall-clock time, more often landing inside one `FixedWindowRateLimiter` window. **Verified on the rebased tree**, pinned SDK 10.0.100: E2E 60/60 twice (~1.85 min each, confirmed against this branch's own freshly spawned subprocesses and Postgres container, not any already-running instance), Architecture 148/148, WebUI 916/920 (4 pre-existing skips), Api 622/622, Infrastructure 360/360, Domain 39/39, Contracts 139/139. Coverage ratchet (Cobertura interface) — WebUI holds at 91.6%, no assembly below baseline.

rob added 2 commits

2026-08-21 11:01:23 +00:00

Wait for Blazor to boot before driving the E2E sign-in flow 17b7e490fd

Regenerate the ADR index table after rebasing onto main

CI / build (pull_request) Successful in 3m44s

Details

CI / container-images (pull_request) Successful in 3m40s

Details

CI / e2e (pull_request) Successful in 4m51s

Details

211676f10e

rob reviewed

2026-08-21 11:13:00 +00:00

rob left a comment

Verdict: changes needed

The core mechanism (App-level ready marker + map-readiness poll, second GotoAsync replaced by an in-app link click) is sound and well-reasoned, but the "closes task 273 suite-wide" claim in both the PR description and ADR-0173's Consequences is false — see the inline comment on JourneySteps.cs. That's a live, reproducible race left in the shipped suite, of exactly the class this PR exists to close, so it blocks.

Verification limits, stated plainly: I could not reproduce the CPU-throttle falsification myself. PlaceMarkAppFixture binds fixed ports (7117/5017/5169), which were already occupied by Rob's own long-running dev API/WebUI processes from the primary working copy (PIDs 2177528/2177783, up since Aug 20). My test run against the worktree completed in a suspicious 1s with no new Testcontainers Postgres container created — it silently rode on the already-running dev instance rather than building and driving this branch's code, and in doing so almost certainly registered a throwaway "Register and Sign-in Journey" account against Rob's live local dev database as a side effect. I did not attempt a second run. Take nothing from that result either way; it isn't evidence for or against this PR.

CI, tied to the head SHA via the checkout line in the run logs: build job succeeded against 211676f10efad073e7aa58d922f7955e6d8d1352; e2e and container-images were still running as of this review (started 11:01:24 UTC, checked at 11:08 — well within this suite's normal ~9 minute total, not stalled). A green run here would still only be necessary, not sufficient, evidence given the RememberViewportJourneyTests gap below.

Coverage question, not blocking on its own: removing the second GotoAsync means no test anywhere now drives a cold GotoAsync straight to /login followed by a local email/password sign-in — OidcSignInJourneyTests does navigate cold to /login, but only exercises the external OIDC button, not the local form. A returning user bookmarking or typing /login directly is a real path that's now untested for local auth. Worth a one-line test or an explicit note that it's accepted, rather than leaving it silently uncovered.

Rate limit change confirmed test-fixture-only (PlaceMarkAppFixture.cs), no production appsettings*.json touched. The 100→200 causal story is plausible given the numbers. It's a magic number tied to current suite size/speed and will likely need raising again as the suite grows — not a reason to block, just don't be surprised.

ADR-0173: numbering, dates, British English and attribution all check out.

Verdict: changes needed The core mechanism (App-level ready marker + map-readiness poll, second GotoAsync replaced by an in-app link click) is sound and well-reasoned, but the "closes task 273 suite-wide" claim in both the PR description and ADR-0173's Consequences is false — see the inline comment on JourneySteps.cs. That's a live, reproducible race left in the shipped suite, of exactly the class this PR exists to close, so it blocks. **Verification limits, stated plainly:** I could not reproduce the CPU-throttle falsification myself. `PlaceMarkAppFixture` binds fixed ports (7117/5017/5169), which were already occupied by Rob's own long-running dev API/WebUI processes from the primary working copy (PIDs 2177528/2177783, up since Aug 20). My test run against the worktree completed in a suspicious 1s with no new Testcontainers Postgres container created — it silently rode on the already-running dev instance rather than building and driving this branch's code, and in doing so almost certainly registered a throwaway "Register and Sign-in Journey" account against Rob's live local dev database as a side effect. I did not attempt a second run. Take nothing from that result either way; it isn't evidence for or against this PR. **CI**, tied to the head SHA via the checkout line in the run logs: `build` job succeeded against `211676f10efad073e7aa58d922f7955e6d8d1352`; `e2e` and `container-images` were still running as of this review (started 11:01:24 UTC, checked at 11:08 — well within this suite's normal ~9 minute total, not stalled). A green run here would still only be necessary, not sufficient, evidence given the RememberViewportJourneyTests gap below. **Coverage question, not blocking on its own:** removing the second `GotoAsync` means no test anywhere now drives a cold `GotoAsync` straight to `/login` followed by a local email/password sign-in — `OidcSignInJourneyTests` does navigate cold to `/login`, but only exercises the external OIDC button, not the local form. A returning user bookmarking or typing `/login` directly is a real path that's now untested for local auth. Worth a one-line test or an explicit note that it's accepted, rather than leaving it silently uncovered. **Rate limit change** confirmed test-fixture-only (`PlaceMarkAppFixture.cs`), no production `appsettings*.json` touched. The 100→200 causal story is plausible given the numbers. It's a magic number tied to current suite size/speed and will likely need raising again as the suite grows — not a reason to block, just don't be surprised. ADR-0173: numbering, dates, British English and attribution all check out.

docs/adr/0173-mark-blazor-ready-for-the-e2e-suite-to-wait-on.md

					
				@ -0,0 +46,4 @@

				reads this marker except the E2E suite; it costs one interop round trip per page load and is

				otherwise inert, so it ships to every reader rather than needing a test-only carve-out. Firing at all

				is itself the guarantee it exists for: `OnAfterRenderAsync` is a JS-to-.NET callback confirming a

				render batch has actually reached the DOM, so by the time it runs, interop has already round-tripped

App.razor wraps routed content in AuthorizeRouteView with an template awaiting a genuine JS-interop round trip (LocalStorageTokenStore) on every navigation, per your own comment there. That means App's first render batch most plausibly renders the "Checking your session…" placeholder, not the routed page — so OnAfterRenderAsync(firstRender: true) doesn't actually guarantee the routed page's own components have rendered yet, contrary to this sentence. It still works in practice, because Blazor's event delegation is wired before/at that first render regardless of what content it carries, and Playwright's own locator auto-wait covers content that appears later — but that's a different, weaker guarantee than "nothing can be present but not yet wired." Worth stating accurately rather than overclaiming.

App.razor wraps routed content in AuthorizeRouteView with an <Authorizing> template awaiting a genuine JS-interop round trip (LocalStorageTokenStore) on every navigation, per your own comment there. That means App's first render batch most plausibly renders the "Checking your session…" placeholder, not the routed page — so OnAfterRenderAsync(firstRender: true) doesn't actually guarantee the routed page's own components have rendered yet, contrary to this sentence. It still works in practice, because Blazor's event delegation is wired before/at that first render regardless of what content it carries, and Playwright's own locator auto-wait covers content that appears later — but that's a different, weaker guarantee than "nothing can be present but not yet wired." Worth stating accurately rather than overclaiming.

docs/adr/0173-mark-blazor-ready-for-the-e2e-suite-to-wait-on.md Outdated

					
				@ -0,0 +118,4 @@

				suite**, not only `MobileTileAndMarkerRetentionJourneyTests`: every test reading `el.placeMarkMap`

				off the map host (`CurrentLocationJourneyTests`, `LeafletControlThemingTests`,

				`DoubleTapDragZoomJourneyTests`, `RememberViewportJourneyTests`, and that class itself) reaches the

				map only after `RegisterAndSignInAsync` returns, so all of them inherit the same guarantee from the

RememberViewportJourneyTests calls page.ReloadAsync() (lines 55, 94) after RegisterAndSignInAsync returns, then reads el.placeMarkMap immediately afterwards with no readiness wait — this class does not "reach the map only after RegisterAndSignInAsync returns" the way this paragraph claims; it reboots again and never re-waits. The suite-wide claim doesn't hold as written.

tests/PlaceMark.E2E.Tests/JourneySteps.cs

					
				@ -34,2 +34,4 @@

						await WaitForAppReadyAsync(page);

						await page.Locator("#register-email").FillAsync(email);

						await page.Locator("#register-display-name").FillAsync(displayName);

This wait only runs inside RegisterAndSignInAsync. RememberViewportJourneyTests.cs:55/94 call page.ReloadAsync() (a full reboot — the same race class this PR fixes) and then read el.placeMarkMap via a one-shot Locator.EvaluateAsync (no retry), guarded only by ToBeVisibleAsync() on the host div — not by WaitForAppReadyAsync or WaitForMapReadyAsync. That's exactly task 273's gap, still open, and it falsifies "closes 273 suite-wide" in both the PR description and ADR-0173's Consequences. Please add the same waits after those ReloadAsync() calls (or wrap ReloadAsync in a helper that always does). MarkerAccessibleNameJourneyTests.cs:43 and RememberGroupVisibilityJourneyTests.cs:42/74 also reload without WaitForAppReadyAsync, but their assertions (ToHaveCountAsync/ToHaveAttributeAsync) auto-retry, so they're not at the same immediate risk — RememberViewportJourneyTests is the one that will actually flake.

rob added 1 commit

2026-08-21 11:37:37 +00:00

Cover every reload, not just GotoAsync, and stop overclaiming in ADR-0173

CI / build (pull_request) Successful in 3m25s

Details

CI / container-images (pull_request) Successful in 3m3s

Details

CI / e2e (pull_request) Successful in 4m22s

Details

69faa64b9c

rob reviewed

2026-08-21 11:45:19 +00:00

rob left a comment

Verdict: changes needed

Round 1's finding is fixed: all five reload sites now route through JourneySteps.ReloadAsync, RememberViewportJourneyTests calls WaitForMapReadyAsync before its one-shot GetCentreAsync reads, and I re-derived the load-bearing claim myself by reading App.razor.cs/LeafletMap.razor.cs/map.js rather than accepting it: the host div.leaflet-map-host is committed to the DOM by Blazor's own render, before OnAfterRenderAsync(firstRender: true) ever awaits createMap's interop call, and element.placeMarkMap is assigned only inside that call (map.js:599). A slow or throttled createMap therefore leaves the div visible with placeMarkMap still undefined — exactly the shape Run #880 and task 273 both describe, and exactly what the guarded/unguarded reads in the described mutation would show. I could not run the mutation experimentally myself: the pinned SDK (global.json, 10.0.100) isn't installed on this host (only 10.0.111 is), and per this repo's own convention a build/test result off an unpinned SDK isn't trustworthy evidence, so I didn't attempt to work around it (no global.json edit — the worktree stays read-only). The structural read is solid enough on its own that I'm not blocking on this point.

New finding: the reboot audit doesn't close the category, it just covers GotoAsync/ReloadAsync. OidcSignInJourneyTests triggers a third kind of reboot this PR doesn't touch: clicking "Sign in with SSO" sends the browser through a real top-level redirect chain (API challenge → stub provider → API callback → back to the WebUI) that lands on a fresh WebUI page exactly as GotoAsync would, but isn't a Playwright GotoAsync/ReloadAsync call so the new wrapper can't and doesn't catch it. Nothing waits for readiness after that click before the two Expect(...).ToBeVisibleAsync() assertions that follow. I checked whether that's actually exposed: this repo sets no SetDefaultTimeout/expect-timeout override anywhere (grepped the whole tree), so those two assertions get Playwright's default 5000ms assertion budget — tighter than the 30000ms action timeout Run #880's original failures already blew through. That's a live gap of the same race class, not a hypothetical one, and it directly contradicts both the Decision section's enumeration ("OidcSignInJourneyTests' one GotoAsync") and the Consequences claim that "every navigation which reboots the runtime... carries the wait." Please add a WaitForAppReadyAsync call after the SSO click (before the two Expect calls), or state explicitly in the ADR why this site is judged low-risk and left uncovered — either is fine, but the current text claims a completeness the code doesn't have.

Vacuous-marker claim: checked directly — no call site invokes WaitForAppReadyAsync after a client-side (in-app) navigation today; the one client-side nav in the suite (Register's "Sign in" link) deliberately doesn't call it, relying on Blazor's event delegation and Playwright's own retry instead, and the doc comments/ADR say so plainly. That's an honest, sufficient guard against someone adding a vacuous wait tomorrow — I wouldn't ask for a redesign (e.g. clearing the attribute on navigation) for a documented edge case this well explained.

Cold sign-in test: SignIn_ReturningUserNavigatingColdToLogin_LandsOnTheAuthenticatedHomePage genuinely exercises a cold boot — fixture.NewPageAsync() opens a fresh, isolated IBrowserContext, so the WASM runtime boots from scratch in that context regardless of any HTTP cache sharing at the browser-process level. Restores real coverage, not just the name of it.

CI: run #882, tied to head 69faa64 via list_workflow_runs. Still build: running, e2e/container-images: blocked, as of 11:44 UTC (started 11:37) — not stalled, just not finished. I'd weigh it as weak evidence either way per the brief, given the suite was already 55/59 green before this fix, so I'm not waiting on it before returning this verdict.

Minor: the PR description is unchanged since round 1 — it still asserts "closes task 273 suite-wide" without the correction ADR-0173's own Consequences section now carries. Worth aligning once the OIDC gap above is settled, so a reader doesn't get a more confident claim from the PR body than from the record it links to.

British English, attribution and ADR numbering/dates all check out.

Verdict: changes needed Round 1's finding is fixed: all five reload sites now route through `JourneySteps.ReloadAsync`, `RememberViewportJourneyTests` calls `WaitForMapReadyAsync` before its one-shot `GetCentreAsync` reads, and I re-derived the load-bearing claim myself by reading `App.razor.cs`/`LeafletMap.razor.cs`/`map.js` rather than accepting it: the host `div.leaflet-map-host` is committed to the DOM by Blazor's own render, before `OnAfterRenderAsync(firstRender: true)` ever awaits `createMap`'s interop call, and `element.placeMarkMap` is assigned only inside that call (`map.js:599`). A slow or throttled `createMap` therefore leaves the div visible with `placeMarkMap` still undefined — exactly the shape Run #880 and task 273 both describe, and exactly what the guarded/unguarded reads in the described mutation would show. I could not run the mutation experimentally myself: the pinned SDK (`global.json`, 10.0.100) isn't installed on this host (only 10.0.111 is), and per this repo's own convention a build/test result off an unpinned SDK isn't trustworthy evidence, so I didn't attempt to work around it (no global.json edit — the worktree stays read-only). The structural read is solid enough on its own that I'm not blocking on this point. **New finding: the reboot audit doesn't close the category, it just covers `GotoAsync`/`ReloadAsync`.** `OidcSignInJourneyTests` triggers a *third* kind of reboot this PR doesn't touch: clicking "Sign in with SSO" sends the browser through a real top-level redirect chain (API challenge → stub provider → API callback → back to the WebUI) that lands on a fresh WebUI page exactly as `GotoAsync` would, but isn't a Playwright `GotoAsync`/`ReloadAsync` call so the new wrapper can't and doesn't catch it. Nothing waits for readiness after that click before the two `Expect(...).ToBeVisibleAsync()` assertions that follow. I checked whether that's actually exposed: this repo sets no `SetDefaultTimeout`/expect-timeout override anywhere (grepped the whole tree), so those two assertions get Playwright's default 5000ms assertion budget — tighter than the 30000ms action timeout Run #880's original failures already blew through. That's a live gap of the same race class, not a hypothetical one, and it directly contradicts both the Decision section's enumeration ("`OidcSignInJourneyTests`' one `GotoAsync`") and the Consequences claim that "every navigation which reboots the runtime... carries the wait." Please add a `WaitForAppReadyAsync` call after the SSO click (before the two `Expect` calls), or state explicitly in the ADR why this site is judged low-risk and left uncovered — either is fine, but the current text claims a completeness the code doesn't have. **Vacuous-marker claim**: checked directly — no call site invokes `WaitForAppReadyAsync` after a client-side (in-app) navigation today; the one client-side nav in the suite (Register's "Sign in" link) deliberately doesn't call it, relying on Blazor's event delegation and Playwright's own retry instead, and the doc comments/ADR say so plainly. That's an honest, sufficient guard against someone adding a vacuous wait tomorrow — I wouldn't ask for a redesign (e.g. clearing the attribute on navigation) for a documented edge case this well explained. **Cold sign-in test**: `SignIn_ReturningUserNavigatingColdToLogin_LandsOnTheAuthenticatedHomePage` genuinely exercises a cold boot — `fixture.NewPageAsync()` opens a fresh, isolated `IBrowserContext`, so the WASM runtime boots from scratch in that context regardless of any HTTP cache sharing at the browser-process level. Restores real coverage, not just the name of it. **CI**: run #882, tied to head `69faa64` via `list_workflow_runs`. Still `build: running`, `e2e`/`container-images`: `blocked`, as of 11:44 UTC (started 11:37) — not stalled, just not finished. I'd weigh it as weak evidence either way per the brief, given the suite was already 55/59 green before this fix, so I'm not waiting on it before returning this verdict. **Minor**: the PR description is unchanged since round 1 — it still asserts "closes task 273 suite-wide" without the correction ADR-0173's own Consequences section now carries. Worth aligning once the OIDC gap above is settled, so a reader doesn't get a more confident claim from the PR body than from the record it links to. British English, attribution and ADR numbering/dates all check out.

docs/adr/0173-mark-blazor-ready-for-the-e2e-suite-to-wait-on.md Outdated

					
				@ -0,0 +154,4 @@

				their own assertions (`ToHaveCountAsync`/`ToHaveAttributeAsync`) auto-retry and so did not immediately

				flake. All five sites now go through `JourneySteps.ReloadAsync`, and `RememberViewportJourneyTests`'s

				own two reads additionally call `WaitForMapReadyAsync` directly before reading `el.placeMarkMap`, the

				same as `RegisterAndSignInAsync` does internally. The suite-wide claim is true now that every

Not quite: OidcSignInJourneyTests reboots a second time when the OIDC redirect chain lands back on the WebUI after the "Sign in with SSO" click, and nothing waits for readiness there — see the inline comment on that file. This sentence, and the Decision section's enumeration above ("OidcSignInJourneyTests' one GotoAsync"), both undercount that test's navigations by one.

Not quite: `OidcSignInJourneyTests` reboots a second time when the OIDC redirect chain lands back on the WebUI after the "Sign in with SSO" click, and nothing waits for readiness there — see the inline comment on that file. This sentence, and the Decision section's enumeration above ("`OidcSignInJourneyTests`' one `GotoAsync`"), both undercount that test's navigations by one.

tests/PlaceMark.E2E.Tests/OidcSignInJourneyTests.cs

This click sends the browser through a real top-level redirect chain (API challenge → stub provider → API callback → back to the WebUI) that reboots the WASM runtime exactly as a GotoAsync would — but it isn't a GotoAsync/ReloadAsync call, so JourneySteps' new wrapper doesn't and can't cover it. Nothing waits for data-app-ready before the two Expect(...).ToBeVisibleAsync() calls below (lines 37, 45), and this repo has no expect-timeout override anywhere, so those get Playwright's default 5000ms budget — tighter than the 30000ms action timeout Run #880's original failures already exceeded. Add await JourneySteps.WaitForAppReadyAsync(page); here, or say explicitly in ADR-0173 why this site is accepted as uncovered.

This click sends the browser through a real top-level redirect chain (API challenge → stub provider → API callback → back to the WebUI) that reboots the WASM runtime exactly as a `GotoAsync` would — but it isn't a `GotoAsync`/`ReloadAsync` call, so `JourneySteps`' new wrapper doesn't and can't cover it. Nothing waits for `data-app-ready` before the two `Expect(...).ToBeVisibleAsync()` calls below (lines 37, 45), and this repo has no expect-timeout override anywhere, so those get Playwright's default 5000ms budget — tighter than the 30000ms action timeout Run #880's original failures already exceeded. Add `await JourneySteps.WaitForAppReadyAsync(page);` here, or say explicitly in ADR-0173 why this site is accepted as uncovered.

rob added 1 commit

2026-08-21 12:05:38 +00:00

Cover the OIDC redirect landing, and enumerate boot paths honestly in ADR-0173

CI / build (pull_request) Successful in 3m40s

Details

CI / container-images (pull_request) Successful in 2m54s

Details

CI / e2e (pull_request) Successful in 4m56s

Details

d726d7ac78

rob reviewed

2026-08-21 12:16:55 +00:00

rob left a comment

Verdict: mergeable

Re-derived the load-bearing claim by execution, not just reading: on this worktree (pinned SDK 10.0.100), with Emulation.setCPUThrottlingRate rate 30 applied and the post-click WaitForAppReadyAsync commented out, SignInWithSso_FirstSignIn_ShowsAddAPlaceWithNoFurtherNavigationOrReload fails at exactly Expect "ToBeVisibleAsync" with timeout 5000ms — waiting for Locator("div.leaflet-map-host") — the identical signature ADR-0173 reports. Restoring the wait under the same throttle passes 3/3. Worktree left clean afterwards.

Enumeration re-audited independently (grep, not trust): no GoBackAsync/GoForwardAsync calls anywhere in the test project, no target="_blank"/window.open in src/PlaceMark.WebUI (the one hit is in a bin/Release build artefact, not source), no window.location assignment in any wwwroot/js file, every <form> in the WebUI carries @onsubmit:preventDefault="true" so no native-POST reboot path exists. Account.razor.cs's link/unlink flows use the same forceLoad: true external-redirect mechanism as Login.razor.cs (mechanism 3) — correctly named as such in the ADR — and no E2E test drives them at all, so there's no uncovered wait to add there; it's the same "no call site" situation as RedirectToSignIn. SignOutAsync navigates without forceLoad, so it's not a reboot and rightly goes unmentioned. I found no fifth boot path.

Verified the "closes task 273 suite-wide" claim by tracing every el.placeMarkMap read in the suite (grep -rn placeMarkMap): every site is downstream of either RegisterAndSignInAsync (waits internally) or RememberViewportJourneyTests' explicit WaitForMapReadyAsync after ReloadAsync. The PR description's current wording ("closed for every place in the suite that reads it") is accurate, not an overclaim — this is the one place round 2 asked for alignment, and it's done.

All five ReloadAsync sites confirmed routed through the wrapper; no raw page.ReloadAsync() remains outside it. All five real GotoAsync sites confirmed to carry the wait.

British English, ADR numbering (0173, no clash with 0171/0172) and attribution all check out; no AI attribution in the diff or recent commits.

CI: run #883, tied to head d726d7ac7858c1a62a2d046fc997ba10307cc7a8 via list_workflow_runs. build and container-images succeeded; e2e was still running at review time (started 12:05:40, checked at 12:13 and again after — within the suite's normal ~9 minute window, not stalled). I'm not waiting on it: the local reproduction above is direct evidence for the one claim that matters, and CI here is corroboration only.

Two inline notes below, neither blocking.

Verdict: mergeable Re-derived the load-bearing claim by execution, not just reading: on this worktree (pinned SDK 10.0.100), with `Emulation.setCPUThrottlingRate` rate 30 applied and the post-click `WaitForAppReadyAsync` commented out, `SignInWithSso_FirstSignIn_ShowsAddAPlaceWithNoFurtherNavigationOrReload` fails at exactly `Expect "ToBeVisibleAsync" with timeout 5000ms — waiting for Locator("div.leaflet-map-host")` — the identical signature ADR-0173 reports. Restoring the wait under the same throttle passes 3/3. Worktree left clean afterwards. Enumeration re-audited independently (grep, not trust): no `GoBackAsync`/`GoForwardAsync` calls anywhere in the test project, no `target="_blank"`/`window.open` in `src/PlaceMark.WebUI` (the one hit is in a `bin/Release` build artefact, not source), no `window.location` assignment in any `wwwroot/js` file, every `<form>` in the WebUI carries `@onsubmit:preventDefault="true"` so no native-POST reboot path exists. `Account.razor.cs`'s link/unlink flows use the same `forceLoad: true` external-redirect mechanism as `Login.razor.cs` (mechanism 3) — correctly named as such in the ADR — and no E2E test drives them at all, so there's no uncovered wait to add there; it's the same "no call site" situation as `RedirectToSignIn`. `SignOutAsync` navigates without `forceLoad`, so it's not a reboot and rightly goes unmentioned. I found no fifth boot path. Verified the "closes task 273 suite-wide" claim by tracing every `el.placeMarkMap` read in the suite (`grep -rn placeMarkMap`): every site is downstream of either `RegisterAndSignInAsync` (waits internally) or `RememberViewportJourneyTests`' explicit `WaitForMapReadyAsync` after `ReloadAsync`. The PR description's current wording ("closed for every place in the suite that reads it") is accurate, not an overclaim — this is the one place round 2 asked for alignment, and it's done. All five `ReloadAsync` sites confirmed routed through the wrapper; no raw `page.ReloadAsync()` remains outside it. All five real `GotoAsync` sites confirmed to carry the wait. British English, ADR numbering (0173, no clash with 0171/0172) and attribution all check out; no AI attribution in the diff or recent commits. **CI**: run #883, tied to head `d726d7ac7858c1a62a2d046fc997ba10307cc7a8` via `list_workflow_runs`. `build` and `container-images` succeeded; `e2e` was still running at review time (started 12:05:40, checked at 12:13 and again after — within the suite's normal ~9 minute window, not stalled). I'm not waiting on it: the local reproduction above is direct evidence for the one claim that matters, and CI here is corroboration only. Two inline notes below, neither blocking.

docs/adr/0173-mark-blazor-ready-for-the-e2e-suite-to-wait-on.md

					
				@ -0,0 +104,4 @@

				   returns to the WebUI.

				4. `NavigationManager.NavigateTo(url, forceLoad: true)` to the *same* origin, called directly by this

				   app's own code rather than reached through an external round trip.

				   `NavigationManagerSignInRedirector.RedirectToSignIn()` — invoked by `RefreshCoordinator` when a

Confirmed: RefreshCoordinator really does call this on refresh-token rejection, and no test in the suite exercises it — verified by grep. Naming it here is sufficient for this PR; worth a follow-up ticket for E2E coverage of the auth-expiry redirect, since it's a real production reboot path of exactly the class this PR fixes. Not blocking.

Confirmed: `RefreshCoordinator` really does call this on refresh-token rejection, and no test in the suite exercises it — verified by grep. Naming it here is sufficient for this PR; worth a follow-up ticket for E2E coverage of the auth-expiry redirect, since it's a real production reboot path of exactly the class this PR fixes. Not blocking.

tests/PlaceMark.E2E.Tests/OidcSignInJourneyTests.cs

Confirmed by execution: with this wait removed and CDP CPU throttling (rate 30) applied, the test fails at 5000ms on this exact locator — matches the ADR's reported reproduction. Restored, it passes 3/3 under the same throttle. Genuinely gates.