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 |
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