fix(home): redirect users without an ID before rendering - #44456
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #892167
Actionable Suggestions - 1
-
superset-frontend/src/pages/Home/index.tsx - 1
- Type contract mismatch · Line 453-463
Additional Suggestions - 1
-
superset-frontend/src/pages/Home/Home.test.tsx - 1
-
Magic route string · Line 186-186The test hardcodes `'/welcome/'` while production uses `RoutePaths.HOME` (index.tsx:459). If the route ever changes, this assertion silently diverges from the constant. Reference `RoutePaths.HOME` so the test tracks the same source of truth.
-
Review Details
-
Files reviewed - 3 · Commit Range:
07c7697..07c7697- docs/docs/quickstart.mdx
- superset-frontend/src/pages/Home/Home.test.tsx
- superset-frontend/src/pages/Home/index.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44456 +/- ##
==========================================
- Coverage 81.71% 81.68% -0.04%
==========================================
Files 2978 2978
Lines 181613 181331 -282
Branches 41983 41928 -55
==========================================
- Hits 148402 148115 -287
- Misses 30489 30490 +1
- Partials 2722 2726 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review Agent Run #2577d7Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 2
Open (5)
useEffect, so asserting immediately afterrender()can be… · NewBoolean(user?.userId)treats0as “missing”, which can incorrectly redirect ifuserIdis ever… · New'/welcome/'while the production code uses a route constant (RoutePaths.*).… · NewhasUserIdis computed but the render path re-checksuser?.userIdinstead of usinghasUserId.… · NewhasUserIdis computed but the render path re-checksuser?.userIdinstead of usinghasUserId.… · New
What changed in this PR
Adds a guard to the Home/Welcome page so SPA navigation for anonymous/guest users triggers a full-page server redirect (instead of crashing when userId is missing), and updates tests/docs accordingly.
Changes:
- Wrap Home with a page-level guard that redirects to the server when
userIdis missing - Add Jest coverage for anonymous/guest/missing-user redirect behavior (and ensure no Home data fetches occur)
- Document the sign-in requirement / redirect behavior in the quickstart
| File | Description |
|---|---|
| superset-frontend/src/pages/Home/index.tsx | Adds a guarded wrapper component that redirects when userId is absent |
| superset-frontend/src/pages/Home/Home.test.tsx | Mocks redirect and adds test cases for anonymous/guest/missing user behavior |
| docs/docs/quickstart.mdx | Notes that personalized Home requires sign-in and redirects from public entrypoints |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Thanks for chasing this down, solid fix for a real crash. CI's green and the Bito thread already got resolved, but a few Copilot comments are still open (the |
|
@rusackas, addressed the remaining Copilot comments in 319ba78. The zero-ID case independently failed before the change; redirect and render now share one explicit null/undefined check. The test awaits the redirect and uses RoutePaths.HOME, while the separate route/navigation suites still cover the literal path and subdirectory behavior. The updated head passes 220 tests and 2 snapshots across 14 suites, plus all applicable pre-commit hooks on the complete three-file PR diff. I did not reproduce the suggested scheduling flake (RTL render already uses act), so I am not claiming that as a confirmed bug. New-head CI is still pending and there was no deployment. Could you take the re-review you offered? CI update: babel-extract fails on the connection-move/OAuth2 warning in messages.pot, not a Home string. The exact missing/stale pair is already covered by open #44547; I am not duplicating its translation changes in this focused Home PR. Other checks are still running, so this head is not CI-green. |
Code Review Agent Run #f786bfActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
@rusackas, could you take another pass? All six original review threads are resolved, including the shared ID-presence guard and the awaited redirect assertion. I merged current Validation on the updated head: 14 suites, 221 tests and 2 snapshots passed; all applicable pre-commit hooks passed, including TypeScript; CI update: |
|
@I3eka Took another pass, this looks solid and all the review threads are resolved. Rerunning the docker-verify job now, that 502 isn't on you. LGTM once CI's green! |
|
Thanks for the re-review and for handling the rerun, @rusackas. All other CI checks on |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Docker verification is green now along with everything else. Approving.
SUMMARY
Prevent the Home page from crashing when an anonymous or guest user reaches it through client-side navigation, such as clicking the Superset logo from the login page or a public dashboard.
The server's
/welcome/view already redirects users without an ID to login. SPA navigation bypasses that view and mountsWelcomewith bootstrap data that has nouserId. The component then throwsTypeError: Cannot read properties of undefined (reading 'toString')before it can render.Add a small page-level guard that renders the personalized Home content only when a user ID exists. Otherwise, use the existing full-page
redirecthelper to request/welcome/, allowing the server to handle login and return navigation. This preserves application-root handling and avoids issuing Home data requests with a missing user ID. Authenticated users retain the existing page and data-loading behavior.Open issues and PRs were checked for the exception,
userId,welcome, and anonymous/unauthenticated navigation before implementing this change. No matching fix was found. Related Home PR #39528 concerns refreshing recent activity after deletion and does not address this crash.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No visual layout changes.
userid!.toString()with the reported exception.TESTING INSTRUCTIONS
Manual reproduction:
/login/in a signed-out browser session using the default Home logo target.Automated validation performed with Node 24.16.0:
cd superset-frontend CI=true NODE_ENV=test NODE_OPTIONS=--max-old-space-size=8192 \ node node_modules/jest/bin/jest.js --runInBand --silent \ src/pages/Home src/pages/Login src/features/home \ src/utils/navigationUtils.test.ts \ src/views/routes.test.tsxResult after merging
master(4cdd680f39) in7696187823: 15 suites, 245 tests and 2 snapshots passed. A new zero-ID case fails with the previous truthiness guard and passes with the explicit null/undefined check; the normal ID and three missing-ID cases are controls. Both redirect and render reuse the same predicate. The effect-driven assertion awaits the redirect and usesRoutePaths.HOME; separate route and navigation tests retain literal path and subdirectory coverage. The original anonymous/guest cases were also run against the unmodified Home component and reproduced the reportedtoStringexception.pre-commit runpassed on all three changed files, including formatting, linting, custom rules, stylelint, and the targeted TypeScript check. The local package declarations were built before that check:python scripts/translations/check_pot_drift.pyalso passed after picking up the upstream catalog fix from #44574. The PR diff remains limited to the same three files.The browser steps above are reproduction instructions, not a claim of a completed browser E2E run. No deployment changes are included.
ADDITIONAL INFORMATION