fix: preserve encoded development proxy URLs#400
Merged
Conversation
Qusic
previously approved these changes
Jul 22, 2026
There was a problem hiding this comment.
Pull request overview
This PR fixes a dev-server fidelity issue where Actix path extraction decoded percent-encoded route parameters before proxying, causing encoded slashes to alter route boundaries and producing invalid upstream URIs for some inputs. The dev server now builds backend state/proxy requests from the original request target (encoded path + query) while keeping decoded paths for local asset lookup, and it documents this transport contract.
Changes:
- Build route-state resolution inputs from
HttpRequest::uri()(encoded path/query) instead of decodedweb::Pathvalues. - Forward
/api/*requests using the original encoded path+query from the incoming request target. - Add regression tests covering encoded spaces,
%2F,%25, and UTF-8 sequences; document the encoded-forwarding contract in user docs andDESIGN.md.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docs/guide/cli/index.md | Documents that --api-port forwards encoded paths/queries unchanged and warns against double-encoding. |
| docs/ai.md | Updates AI/CLI reference to reflect the encoded-forwarding behavior under --api-port. |
| DESIGN.md | Specifies the encoded-path/query preservation contract for dev-server proxying and route state requests. |
| crates/webui-cli/src/commands/serve.rs | Uses the original request target for route-state fetch and /api/* proxying; adds wire-level regression tests. |
Development requests with encoded route parameters were decoded by Actix path extraction before forwarding. Encoded slashes changed route boundaries, while spaces and UTF-8 values could produce invalid upstream URIs, leading to partial path mismatches or fallback state. Build backend requests from the original URI path and query for both route-state resolution and /api forwarding, while retaining decoded paths for local asset lookup. Add wire-level regression coverage and document the transport contract. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve the raw request URI behavior while restoring explicit capacity planning in handle_api_proxy. Append the origin, port, and encoded request target into a pre-sized buffer instead of formatting a new URL on every proxied request. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The dev-server SPA fallback decided whether a request was a static asset by testing the Actix-decoded path for a literal '.'. An encoded dot (%2E) in a route parameter decoded to '.', so encoded-dot routes were wrongly treated as file requests and returned 404 instead of resolving backend state, contradicting the encoded-URI preservation this PR added. Test the original encoded request path for the extension heuristic so %2E stays a route-parameter byte, and forward it to the backend unchanged. Add a regression test covering an encoded-dot route parameter and a genuinely missing asset. Also document the one intentional exception to the "encoded path forwarded exactly" contract: the entry route alias. Both / and /index.html resolve backend state at / (the entry path is normalized) while preserving the query string. Updated DESIGN.md, the CLI guide, and the AI reference so the transport contract is accurate. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
mohamedmansour
force-pushed
the
mohamedmansour-fix-dev-proxy-url-encoding
branch
from
July 22, 2026 22:23
bc92d4e to
45f8f98
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
crates/webui-cli/src/commands/serve.rs:982
spa_fallbacktreats any dot anywhere in the path as a file-extension indicator. That misclassifies valid route paths like/v2.1/usersor/folder.with.dot/profileas assets and returns 404 even though only the last segment needs to look like a filename. Check the final segment of the encoded path instead.
if paths.route_path.contains('.') {
The `build (examples)` step builds every example app in parallel, and each app's `build:deps` recompiles the shared workspace packages (@microsoft/webui, @microsoft/webui-framework, @microsoft/webui-router) with `tsc` into their shared `dist/` directories. Running those tsc invocations concurrently races on the shared output: one app reads packages/webui/dist/projection/adapters/esbuild.js while another app's tsc is mid-rewrite of the sibling loader.js, surfacing in CI as "SyntaxError: does not provide an export named 'compileProjection'". Seed the shared packages once before the parallel app builds (ensure_example_deps, mirroring ensure_example_wasm) and enable tsc `incremental` on the three shared tsconfigs so each per-app build:deps becomes a no-op emit that never rewrites the shared files concurrently. The incremental buildinfo is gitignored. Verified: six concurrent tsc runs against a seeded tree produce zero re-emit. This is strictly less work than before, where the shared build ran once per app in parallel. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
pnpm honours the root `packageManager` pin by re-running its package-manager self-management on every `pnpm run`, resolving a pinned pnpm under ~/AppData/Local/pnpm/.tools. On a machine whose shared global pnpm store is being churned by other long-running pnpm processes, that pre-run step blocks for tens of seconds before the script even starts, even though the underlying work (e.g. the electron esbuild integration build) takes milliseconds. This made `cargo xtask check` appear to hang after `integration: electron`. Set `npm_config_manage_package_manager_versions=false` for pnpm commands in `build_command`, mirroring the existing `CARGO_INCREMENTAL=0` handling for cargo. CI and the engines/packageManager fields already guarantee the correct pnpm, so xtask runs the already-resolved binary directly. The electron integration drops from a 90s+ stall to 0.1s and the full build-examples phase completes in ~25s. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The `--api-port` help text claimed encoded path and query bytes are forwarded unchanged, but the entry-route alias resolves backend state for both `/` and `/index.html` at `/` (query preserved). Note that exception in the CLI help so `webui serve --help` matches the behavior already documented in docs and DESIGN.md. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Qusic
previously approved these changes
Jul 23, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Parse Accept q weights before selecting SPA fallback responses so q=0 prevents HTML or JSON fallback and higher weights win deterministically. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Out-of-range but parseable q values (e.g. q=2 or q=-1) are malformed per HTTP qvalue semantics. Treat them as absent with the default q of 1.0 so an invalid Accept header cannot force a 404 or skew HTML/JSON tie-breaking. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The docs claimed invalid Accept headers return 404, but a malformed or out-of-range q value is treated as the default q=1.0 (still acceptable). Drop the inaccurate 'invalid' wording and document the q=1.0 fallback so DESIGN.md and the guide match webui serve behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
crates/webui-cli/src/commands/serve.rs:1181
handle_api_proxystill allocates a temporaryStringviaapi_port.to_string()on every proxied request. This defeats some of the capacity planning here and adds avoidable work on the proxy hot path.
You can write the port directly into the already-allocated url buffer (infallible for String) and then append the path/query in the same formatting call.
let mut url =
String::with_capacity(API_URL_PREFIX.len() + MAX_PORT_DIGITS + path_and_query.len());
url.push_str(API_URL_PREFIX);
url.push_str(&api_port.to_string());
url.push_str(path_and_query);
handle_api_proxy reserved MAX_PORT_DIGITS of capacity for the port but then allocated a throwaway String via api_port.to_string() on every proxied request. Write the digits directly into the buffer with write!, matching the capacity planning and the crate's existing buffer-write convention. Addresses a low-confidence proxy hot-path review suggestion. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Qusic
approved these changes
Jul 23, 2026
atulkatti
approved these changes
Jul 23, 2026
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.
Development requests with encoded route parameters were decoded by Actix path extraction before forwarding. Encoded slashes changed route boundaries, while spaces and UTF-8 values could produce invalid upstream URIs, leading to partial path mismatches or fallback state.
Build backend requests from the original URI path and query for both route-state resolution and /api forwarding, while retaining decoded paths for local asset lookup. Add wire-level regression coverage and document the transport contract.
Bundled build-system fixes
While validating this change through
cargo xtask check, two unrelated but blocking build-system issues were fixed here to keep the gate and CI green. They are called out separately so the scope stays clear:tscexample-build race (ensure_example_deps+ TypeScriptincremental): each example app'sbuild:depsrecompiled the shared JS packages into the samedist/concurrently, so one app could read a half-writtenloader.js/esbuild.jswhile another app'stscrewrote it, surfacing in CI asdoes not provide an export named 'compileProjection'. The shared packages are now built once up front and the shared tsconfigs are incremental, so each per-appbuild:depsis a no-op emit.npm_config_manage_package_manager_versions=falsein xtask'sbuild_command): everypnpm runre-ran pnpm's version self-management, which could block for tens of seconds before the script started on a machine whose shared pnpm store was busy. xtask now runs the already-resolved pnpm directly (engines/packageManagerand CI already pin the version).