Skip to content

fix: prevent setup wizard redirect on 404 routes#38407

Open
Arnab-nandi-10 wants to merge 8 commits intoRocketChat:developfrom
Arnab-nandi-10:fix/stop-redirect-on-404
Open

fix: prevent setup wizard redirect on 404 routes#38407
Arnab-nandi-10 wants to merge 8 commits intoRocketChat:developfrom
Arnab-nandi-10:fix/stop-redirect-on-404

Conversation

@Arnab-nandi-10
Copy link

@Arnab-nandi-10 Arnab-nandi-10 commented Jan 28, 2026

Fixes #38377

Problem

When opening an invalid route (e.g. /abc123) during setup wizard state,
the app briefly shows the 404 page and then redirects to /setup-wizard.

Expected behavior

The app should remain on the 404 page and not redirect.

Solution

Prevented useRedirectToSetupWizard from redirecting
when the current route is not-found.

This change is intentionally minimal and only affects redirection logic for invalid routes.

How to test

  1. Ensure Show_Setup_Wizard is pending or in_progress
  2. Open /random-invalid-route
  3. Verify the app stays on the 404 page (no redirect)

Demo

🎥 Video attached

Recording.2026-01-29.005750.mp4

Summary by CodeRabbit

  • Bug Fixes
    • Improved setup wizard redirect behavior: now waits for router and setup state before acting, avoids redirecting on not-found pages, more reliably detects when the wizard is in progress, and only triggers redirects when guarded conditions are met to prevent unintended navigation.

✏️ Tip: You can customize this high-level summary in your review settings.

@Arnab-nandi-10 Arnab-nandi-10 requested a review from a team as a code owner January 28, 2026 19:59
@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Jan 28, 2026

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label
  • This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Jan 28, 2026

⚠️ No Changeset found

Latest commit: b6f14ae

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 28, 2026

Walkthrough

Reworks the setup-wizard redirect hook to wait for router and setupWizardState, derive the current routeName to detect not-found pages, coerce wizard-state booleans, and only perform the redirect when guarded conditions pass (avoiding redirects from 404 routes). Effect dependencies were updated.

Changes

Cohort / File(s) Summary
Setup Wizard Redirect Hook
apps/meteor/client/views/root/hooks/useRedirectToSetupWizard.ts
Replaced immediate redirect with guarded effect: early-return if router or setupWizardState missing, derive routeName and isNotFoundPage, coerce isWizardInProgress via Boolean(...), compute mustRedirect and only redirect when not on a not-found page; expanded effect dependencies and minor formatting.

Sequence Diagram(s)

sequenceDiagram
    participant Client as Client
    participant Router as Router
    participant Hook as useRedirectToSetupWizard
    participant State as SetupWizardState

    Client->>Router: navigate to URL
    Router->>Hook: provide router (with routeName)
    Hook->>State: read setupWizardState, userId, isAdmin
    Hook->>Hook: compute isNotFoundPage, isWizardInProgress, mustRedirect
    alt mustRedirect && not isNotFoundPage
        Hook->>Router: redirect to /setup-wizard
    else
        Hook-->>Client: allow current route (including 404)
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hop the routes both near and wide,
I wait for signs before I guide,
If a path is lost and says "not found,"
I leave it be on safe, soft ground,
No wizard leap where ghosts reside.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'fix: prevent setup wizard redirect on 404 routes' directly and clearly summarizes the main change: preventing unwanted redirects to the setup wizard when users navigate to invalid routes that display 404 pages.
Linked Issues check ✅ Passed The code changes successfully address all core requirements from issue #38377: preventing redirect to /setup-wizard on invalid routes, allowing 404 pages to display, and maintaining correct redirects on valid routes during setup state.
Out of Scope Changes check ✅ Passed All changes in the PR are directly scoped to the stated objectives of fixing the setup wizard redirect behavior on 404 routes; no unrelated modifications are present.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Warning

Review ran into problems

🔥 Problems

Errors were encountered while retrieving linked issues.

Errors (1)
  • ON-404: Request failed with status code 404

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 1 file

Copy link
Member

@MartinSchoeler MartinSchoeler left a comment

Choose a reason for hiding this comment

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

This is a very shallow fix that only patches the symptom of the bug, we need to find the actual root case instead of only creating an exception on the redirect system

@Arnab-nandi-10
Copy link
Author

This is a very shallow fix that only patches the symptom of the bug, we need to find the actual root case instead of only creating an exception on the redirect system

Thank You for the feedback @MartinSchoeler,

I’ve updated the redirect logic to avoid undefined router access and added
additional guards for routeName and setupWizardState.

I also re-tested:

  • Pending state → invalid route stays on 404
  • In-progress state → invalid route stays on 404
  • Valid routes still redirect correctly

I also test in many several ways.
Please let me know if you’d like any further refinement.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@apps/meteor/client/views/root/hooks/useRedirectToSetupWizard.ts`:
- Around line 11-26: Remove the implementation-level inline comments in
useRedirectToSetupWizard (the guards and condition explanations) so the code is
self-describing; specifically delete/comment-out lines that annotate the guards,
"Current route", "Conditions", and "Redirect only if needed" around the
variables router, setupWizardState, routeName, isNotFoundPage,
isWizardInProgress, and mustRedirect, leaving only the code logic in the
useRedirectToSetupWizard hook.
- Around line 15-27: The redirect logic in useRedirectToSetupWizard uses
router.getRouteName() coalesced to '' which hides the unresolved-routing case
and causes premature redirects; update the guard so that if
router.getRouteName() returns undefined you early-bail (do not proceed with
redirect checks), e.g. detect routeName === undefined before computing
isNotFoundPage/mustRedirect and return (or skip) when undefined; adjust the
variables referenced (routeName, getRouteName(), isNotFoundPage, mustRedirect)
accordingly so redirects only run when a concrete route name is available.

@Arnab-nandi-10
Copy link
Author

@MartinSchoeler I would love to know if there is anything i need to change , i already tried many test cases,looking forward to ur response.

@Arnab-nandi-10
Copy link
Author

@MartinSchoeler It looks like the QA check is failing due to missing milestone and stat: QA assured label.
Could you please help assign the appropriate milestone and labels when possible?
Thank you!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Redirection of error page(Random route) to a setup wizard page

3 participants