fix: prevent setup wizard redirect on 404 routes#38407
fix: prevent setup wizard redirect on 404 routes#38407Arnab-nandi-10 wants to merge 8 commits intoRocketChat:developfrom
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughReworks the setup-wizard redirect hook to wait for Changes
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
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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. Comment |
MartinSchoeler
left a comment
There was a problem hiding this comment.
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 I also re-tested:
I also test in many several ways. |
There was a problem hiding this comment.
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.
apps/meteor/client/views/root/hooks/useRedirectToSetupWizard.ts
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/root/hooks/useRedirectToSetupWizard.ts
Outdated
Show resolved
Hide resolved
|
@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. |
|
@MartinSchoeler It looks like the QA check is failing due to missing milestone and |
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
useRedirectToSetupWizardfrom redirectingwhen the current route is
not-found.This change is intentionally minimal and only affects redirection logic for invalid routes.
How to test
Show_Setup_Wizardispendingorin_progress/random-invalid-routeDemo
🎥 Video attached
Recording.2026-01-29.005750.mp4
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.