Skip to content

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

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

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

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 the '/app' endpoint and adds a corresponding test case to verify the behavior. The reviewer suggests using a 301 Moved Permanently status code instead of 302 Found to improve SEO and caching, along with updating the test assertions to match this change.


@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

Using 302 Found indicates a temporary redirect. Given that the root is being redirected to the main application entry point, this is likely a permanent change. It's better practice to use a permanent redirect status code like 301 Moved Permanently. This helps with SEO and allows browsers and other clients to cache the redirect.

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

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 suggested change of using a 301 status code for the permanent redirect, this assertion should be updated.

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

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