fix: harden router navigation lifecycle - #439
Open
Mohamed Mansour (mohamedmansour) wants to merge 4 commits into
Open
fix: harden router navigation lifecycle#439Mohamed Mansour (mohamedmansour) wants to merge 4 commits into
Mohamed Mansour (mohamedmansour) wants to merge 4 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 79ab856c-34d8-40c0-9d0d-7d21125739bb
Mohamed Mansour (mohamedmansour)
requested review from
a team,
Jane Chu (janechu) and
mcritzjam
and
a lite review from Copilot
August 13, 2026 04:28
Copilot started reviewing on behalf of
Mohamed Mansour (mohamedmansour)
August 13, 2026 04:29
View session
There was a problem hiding this comment.
Pull request overview
Hardens @microsoft/webui-router navigation lifecycle to avoid stalled navigations, incorrect cache completion for aborted streams, and pending/error UI being selected from (or mounted into) the route being replaced. The change aligns runtime behavior with the updated internal lifecycle contract in DESIGN.md.
Changes:
- Add a bounded 10s deadline for the initial partial payload (JSON parse or first NDJSON chain chunk), plus router-owned abort controllers that also cover deferred NDJSON readers.
- Guard async boundaries with navigation/boundary generations to prevent stale pending/error mounts and stale side effects.
- Introduce destination-boundary selection (
route-boundary.ts) that walks SSR route placeholders to choose pending/error hints from the destination hierarchy and mount them into the correct shared outlet.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/webui-router/src/router.ts | Adds partial fetch deadline + controller tracking, abort-on-supersede/destroy, and generation guards; switches pending/error selection to destination-boundary logic. |
| packages/webui-router/src/streaming.ts | Prevents marking cache entries complete unless the deferred stream reaches clean EOF; adds post-read generation/abort check. |
| packages/webui-router/src/pending.ts | Updates pending/error mounting to target a provided container and restores prior route display state on cleanup. |
| packages/webui-router/src/route-boundary.ts | New destination pending/error boundary selector based on SSR-emitted route placeholders and path specificity rules. |
| packages/webui-router/src/route-boundary.test.ts | Unit tests for boundary path splitting/matching (optional params, splats, exactness, query stripping, unsafe param rejection). |
| packages/webui-router/src/router.test.ts | Expands unit coverage around abort propagation, bounded timeouts (pre-headers and during JSON read), and superseding navigation aborting deferred streams without completing cache entries. |
| packages/webui-router/tests/router-e2e.spec.ts | Adds e2e coverage for destination pending/error boundaries after a prior route is active and for literal-over-parameter specificity. |
| packages/webui-router/tests/fixtures/router-app/src/index.html | Extends fixture route config to create literal vs parameter destination boundary cases and pending boundary coverage. |
| DESIGN.md | Documents the internal lifecycle contract for bounded initial payload, router-owned cancellation for deferred readers, and destination boundary selection behavior. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 79ab856c-34d8-40c0-9d0d-7d21125739bb
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 79ab856c-34d8-40c0-9d0d-7d21125739bb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary This PR closes four router lifecycle gaps that can leave a navigation stalled, cache partial content as complete, or render pending/error UI in the route being replaced: - bounds the initial partial response through JSON parsing or the first NDJSON chain chunk with a cancellable 10-second deadline; - keeps router-owned cancellation alive for deferred readers and aborts every normal or speculative stream when a newer navigation supersedes it or the router is destroyed; - prevents delayed pending imports and deferred reads from committing after their navigation generation is stale; - selects pending/error hints from the destination route hierarchy, mounts them in the exact shared outlet, and restores the prior route display state during cleanup. This is transparent runtime correctness. It does not add application shims, new public APIs, or user-facing documentation. ## Why this is needed ### A response is not complete when
fetch()resolvesfetch()resolves once response headers arrive. Previously, JSON parsing and the first NDJSON read had no deadline, so a server that sent headers and then stalled could hold navigation indefinitely. The deadline now remains active until the router has a usable initial payload, and it is cancelled immediately on the fast path. ### A completed navigation signal cannot own a deferred stream Deferred NDJSON work continues after the initial route chain mounts. The originatingNavigateEvent.signalmay remain live after that transition completes, so a later navigation cannot reliably interrupt a stalledreader.read(). Router-owned controllers now remain registered only while a request or deferred reader is active. A new non-speculative navigation ordestroy()aborts the whole set. The implementation usesAbortSignal.any()rather than manually forwarding abort events. That preserves the originating abort reason and adds zero application-level forwarding listeners or cleanup closures. ### Async boundaries need generation guards, not timer cleanup alone Clearing a pending timer cannot cancel a dynamicimport(), and cancelling a stream can race with an already-resolvedreader.read(). The router now validates both navigation and boundary generations after every asynchronous handoff. Stale work cannot mount pending UI, register templates/styles, or mark an incomplete cache entry complete. ### Pending/error UI must belong to the destination The old lookup started from the active chain, which can point at the route being removed. The new internal selector walks SSR-emitted route placeholders top-down, prefers literals over parameters, supports optional/splat/exact paths, carries the actual outlet container, and fails closed on equal-specificity ambiguity instead of trusting reordered SSR DOM. The server-provided route chain remains authoritative for content. ## Memory and performance Measurements were taken in Chromium against this commit: | Metric | Result | | --- | ---: | | Destination-boundary lookup, 32 sibling routes, 25 samples x 20,000 lookups | 4.73 microseconds median, 4.96 microseconds p95 | | Detached route retention after 5,000 error mount/clear cycles (15,000 route nodes), forced GC | 0 retained | | Manual abort-forwarding listeners per request | 0 | Production-style minified ESM bundle, with@microsoft/webui-frameworkexternalized: | Size |origin/main| This PR | Delta | | --- | ---: | ---: | ---: | | Raw | 28,189 B | 30,695 B | +2,506 B (+8.89%) | | Gzip | 9,146 B | 10,003 B | +857 B (+9.37%) | | Brotli | 8,190 B | 8,948 B | +758 B (+9.26%) | The lifecycle cost is bounded to one controller, one cancellable timer, and oneSetentry per active initial request. The timer is cleared once the first usable payload arrives; the controller entry is removed immediately unless a deferred reader still owns it, then removed at clean EOF or abort. Error-route display tracking is allocated only while an error boundary is mounted and is released during cleanup. ## Coverage - navigation abort reasons and already-aborted signals; - deadlines that expire before headers, during JSON parsing, and before the first NDJSON chain chunk; - normal and speculative deferred cancellation; - cache entries remaining incomplete after aborted streams; - post-read generation races; - delayed pending imports, superseding navigation, and router destruction; - destination pending/error boundaries after another route is active; - exact shared-outlet placement and route display restoration; - literal-over-parameter specificity and equal-specificity fail-closed behavior; - malformed encoded parameters and server/client boundary-matching parity. Validation: -pnpm --dir packages/webui-router test: 72 unit tests passed; 28 browser tests passed; 4 capability-gated tests skipped -cargo xtask check: all phases passed, including formatting, Clippy, dependency audit, workspace tests, native/WASM/example builds, benchmark validation, and docs build ## Documentation OnlyDESIGN.mdis updated to record the internal lifecycle contract. No files underdocs/are changed because applications do not need new authoring guidance or configuration.