Skip to content

Add root '/' redirect to /app and corresponding test#11

Open
mrauter1 wants to merge 1 commit intomainfrom
codex/fix-404-error-on-root-route-8yaii8
Open

Add root '/' redirect to /app and corresponding test#11
mrauter1 wants to merge 1 commit intomainfrom
codex/fix-404-error-on-root-route-8yaii8

Conversation

@mrauter1
Copy link
Copy Markdown
Owner

Motivation

  • Ensure requests to the site root redirect browsers to the single-page app at /app for a smoother entry point.

Description

  • Add a GET handler for / in app/main.py that returns RedirectResponse("/app", status_code=status.HTTP_302_FOUND) and import status and RedirectResponse.
  • Add a unit test test_root_route_redirects_to_app in tests/test_auth_requester.py to validate the redirect.

Testing

  • Added tests/test_auth_requester.py::test_root_route_redirects_to_app and ran the test suite; the new test passed with the existing tests.

Codex Task

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a redirect from the root path to /app and adds a corresponding test case. The reviewer suggests using a 308 Permanent Redirect instead of a 302 Found status code to allow for browser caching and maintain the request method, along with a necessary update to the test assertion.


@app.get("/", include_in_schema=False)
def root_redirect() -> RedirectResponse:
return RedirectResponse("/app", status_code=status.HTTP_302_FOUND)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

For a permanent redirect like this, it's better to use a permanent redirect status code. This can improve performance for repeat visitors as browsers can cache the redirect. I suggest using 308 Permanent Redirect.

The 308 status code preserves the request method (GET in this case) and indicates the redirect is permanent. You will also need to update the corresponding test to expect a 308 status code.

Suggested change
return RedirectResponse("/app", status_code=status.HTTP_302_FOUND)
return RedirectResponse("/app", status_code=status.HTTP_308_PERMANENT_REDIRECT)

with stack["TestClient"](app, base_url="https://testserver") as client:
response = client.get("/", follow_redirects=False)

assert response.status_code == 302
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

To align with the recommended change of using a permanent redirect in app/main.py, this assertion should check for a 308 status code instead of 302.

Suggested change
assert response.status_code == 302
assert response.status_code == 308

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant