Skip to content

Fix 404 handler swallowing SPA fallback under ui.run_with(root=...)#5999

Merged
evnchn merged 2 commits intomainfrom
fix/404-handler-mount-endpoint-fallthrough
Apr 25, 2026
Merged

Fix 404 handler swallowing SPA fallback under ui.run_with(root=...)#5999
evnchn merged 2 commits intomainfrom
fix/404-handler-mount-endpoint-fallthrough

Conversation

@falkoschindler
Copy link
Copy Markdown
Contributor

Motivation

Closes #5998.

#5974 changed the 404 handler to return JSON when an endpoint matched but no nicegui_page_path was set, so that @app.get routes raising HTTPException(404) would no longer be served NiceGUI's HTML error page. The discriminator was 'endpoint' in request.scope.

That works under plain ui.run(). Under ui.run_with(parent_app, root=root) it breaks SPA fallback: Starlette's Mount.matches() populates scope["endpoint"] with the mounted app reference before the inner router runs (see starlette/routing.py L429). When the inner router doesn't match anything (e.g. /auth/login is only known to a client-side ui.sub_pages), that endpoint survives into the 404 handler. The new branch then incorrectly fires and returns {"detail":"Not Found"} instead of falling through to render root.

Implementation

Tighten the discriminator: only treat the request as "an endpoint raised 404" when scope["endpoint"] is set and is not core.app itself. A real inner Route match overwrites endpoint with the handler function, so this distinguishes the two cases cleanly.

Added two regression tests in tests/test_run_with.py exercising the mounted-app code path that the existing tests/test_page.py tests don't cover:

Progress

  • The PR title is a short phrase starting with a verb like "Add ...", "Fix ...", "Update ...", "Remove ...", etc.
  • The implementation is complete.
  • This PR does not address a security issue.
  • Pytests have been added.
  • Documentation is not necessary.
  • No breaking changes to the public API.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@falkoschindler falkoschindler added bug Type/scope: Incorrect behavior in existing functionality review Status: PR is open and needs review 🔴 critical Priority: Needs immediate attention labels Apr 25, 2026
@falkoschindler falkoschindler added this to the 3.11.1 milestone Apr 25, 2026
@falkoschindler falkoschindler requested review from evnchn and rodja April 25, 2026 16:07
Drops the intermediate `endpoint = request.scope.get('endpoint')` line
and binds inline with `(endpoint := ...) is not None`. Behavior is
identical (Starlette's `Mount.matches` and `Route.matches` always
write a non-None value to `scope['endpoint']`); this just trims the
diff to one line.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@evnchn
Copy link
Copy Markdown
Collaborator

evnchn commented Apr 25, 2026

TL;DR

Reviewed empirically (cloned, ran tests on main + on this branch under both Starlette 1.0.0 and 0.52.1, walked the discriminator across 7 scenarios) and against Starlette source. Fix is correct, surgical, and the new tests fail on main as a regression guard should. Safe to merge.

I pushed one tiny commit on top (4e84367) that uses a walrus to drop the intermediate endpoint = line — purely cosmetic, takes the diff from +2 -1 to +1 -1. If you don't like walrus in a long boolean, just revert that one commit — the bare-'endpoint' in scope form (no walrus, no local var) is also a valid minimum-diff alternative. I'm pushing for walrus because the chance to use one is easy to miss in review and trivial to remove later.

Full review — un-blackboxing Starlette + variant comparison

Starlette code path, cited

When parent_app.mount('/', core.app) (per nicegui/ui_run_with.py:114):

  1. Outer router runs Mount.matches() (starlette 1.0.0/routing.py L417, 0.52.1/routing.py L429). The matched child_scope includes "endpoint": self.app — i.e. our core.app instance.
  2. Outer Router.app does scope.update(child_scope) (1.0.0 L676) before entering the mount.
  3. Inner router walks its own routes. None match /auth/login. It calls self.default() which raises HTTPException(status_code=404) (1.0.0 L616).
  4. Our _exception_handler_404 runs. scope['endpoint'] is still core.app from step 1.

Contrast: when a real inner Route matches, Route.matches() (1.0.0 L249) writes "endpoint": self.endpoint (the handler function), overwriting the Mount's core.app value. So endpoint is app cleanly distinguishes the two cases. Identity check (not ==) is the right operator. Same line numbers and behavior on 0.52.1.

Cross-version verification

Reproduced regression on main and verified the fix on the branch under both versions independently:

Stack main (4 tests, with PR's tests merged in) This branch
fastapi==0.119.1 + starlette==1.0.0 1 failed, 3 passed 4/4 pass
fastapi==0.130.0 + starlette==0.52.1 1 failed, 3 passed 4/4 pass

Same failing assertion in both: /auth/login returns application/json 404 instead of text/html root.

Empirical truth table (7 scenarios)

Verified across main, your original commit, walrus, and the non-walrus minimal form. All three fixes behave identically; only main differs, and only on row D.

# Scenario 'endpoint' in scope scope['endpoint'] value nicegui_page_path Needed main All 3 fixes
A plain ui.run(), unknown URL HTML root HTML ✓ HTML ✓
B plain ui.run(), @app.get→404 handler fn JSON JSON ✓ JSON ✓
C plain ui.run(), @ui.page→404 handler fn HTML err HTML ✓ HTML ✓
D run_with, unknown URL (#5998) app HTML root JSON ✗ HTML ✓
E run_with, inner @app.get→404 handler fn JSON JSON ✓ JSON ✓
F run_with, @ui.page→404 handler fn HTML err HTML ✓ HTML ✓
G run_with, static asset missing StaticFiles JSON JSON ✓ JSON ✓

Also verified mount_path='/gui' works: /gui/, /gui/auth/login render the root page; /gui/totally-unknown falls through; /notmounted returns the outer FastAPI's default JSON.

The three forms — pick one

A. Two-line / explicit local (your original commit):

endpoint = request.scope.get('endpoint')
if endpoint is not None and endpoint is not app and not request.scope.get('nicegui_page_path') and isinstance(exception, StarletteHTTPException):

B. Walrus (what I just pushed in 4e84367):

if (endpoint := request.scope.get('endpoint')) is not None and endpoint is not app and not request.scope.get('nicegui_page_path') and isinstance(exception, StarletteHTTPException):

C. Non-walrus minimal — preserves the existing 'endpoint' in scope idiom:

if 'endpoint' in request.scope and request.scope['endpoint'] is not app and not request.scope.get('nicegui_page_path') and isinstance(exception, StarletteHTTPException):
Variant Δ vs main (logic) New syntax scope lookups Notes
A — current +2 −1 none 2 Most readable
B — walrus (pushed) +1 −1 := 2 Single line
C — bare in scope +1 −1 none 3 Closest to original idiom; one extra dict lookup nobody will notice

Equivalence: Starlette only ever writes a non-None value into scope['endpoint'] (Mount: self.app, Route: self.endpoint), so 'endpoint' in scope ⇔ scope.get('endpoint') is not None. All three guards are identical Boolean functions of the scope.

If walrus-in-a-long-boolean reads worse to you than 'endpoint' in scope, revert 4e84367 and switch to form C — that's the bare-minimum diff with no new syntax.

Copy link
Copy Markdown
Collaborator

@evnchn evnchn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TL-DR: The previous report means a LGTM

@falkoschindler falkoschindler added this pull request to the merge queue Apr 25, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Apr 25, 2026
@evnchn evnchn added this pull request to the merge queue Apr 25, 2026
Merged via the queue into main with commit 31c4a69 Apr 25, 2026
7 checks passed
@evnchn evnchn deleted the fix/404-handler-mount-endpoint-fallthrough branch April 25, 2026 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Type/scope: Incorrect behavior in existing functionality 🔴 critical Priority: Needs immediate attention review Status: PR is open and needs review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ui.run_with(FastAPI(), root=...) no longer resolves SPA subpaths correctly in 3.11.0

2 participants