Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix incorrect loading of admin ui settings page #2805

Merged
merged 3 commits into from
Jul 15, 2024

Conversation

JoblersTune
Copy link
Collaborator

@JoblersTune JoblersTune commented Jul 11, 2024

Changes proposed in this pull request

  • This PR addresses a bug in the Admin UI where the settings page fails to load correctly, resulting in a 404 error. The issue arises due to the browser getting stuck at the Kratos endpoint and not following the redirect back to the settings page in the Admin UI with the flow ID appended. This bug is resolved by replacing the redirect function with redirectDocument to ensure the correct handling of document-level redirects. In the settings.tsx page I'm simply swapping redirect with redirectDocument.

We are using a nightly build to test if this resolves issues in possible production deployments.

Context

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Documentation added
  • Make sure that all checks pass
  • Bruno collection updated

@JoblersTune JoblersTune requested a review from golobitch July 11, 2024 06:21
@JoblersTune JoblersTune self-assigned this Jul 11, 2024
@github-actions github-actions bot added the pkg: frontend Changes in the frontend package. label Jul 11, 2024
Copy link

netlify bot commented Jul 11, 2024

Deploy Preview for brilliant-pasca-3e80ec canceled.

Name Link
🔨 Latest commit 50c723d
🔍 Latest deploy log https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/6694d374a1fb130008834367

mkurapov
mkurapov previously approved these changes Jul 12, 2024
@@ -39,7 +43,7 @@ export const loader = async ({ request }: LoaderFunctionArgs) => {

return { responseData }
} else {
return redirect(
return redirectDocument(
Copy link
Contributor

Choose a reason for hiding this comment

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

is this the only place this is needed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We only found issue with the settings page.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mkurapov is actually a very good question. It made me realise that we need to make improvements on catching direct references to the login page and account recovery page as well! Both were already redirecting to the kratos endpoint when the login / Forgot password? buttons were clicked but weren't handling direct navigation optimally and should also be using redirectDocument if they are navigated to directly instead of just throwing an error.

I'm going to include an additional commit now that covers this as well.

golobitch
golobitch previously approved these changes Jul 12, 2024
@JoblersTune JoblersTune dismissed stale reviews from golobitch and mkurapov via f3175af July 15, 2024 07:38
@JoblersTune JoblersTune merged commit 9af1dcb into main Jul 15, 2024
36 of 42 checks passed
@JoblersTune JoblersTune deleted the 2793/sj/remix-reload-issues branch July 15, 2024 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: frontend Changes in the frontend package.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] The settings page is not loading for the Admin UI
3 participants