Copilot commented on code in PR #44456:
URL: https://github.com/apache/superset/pull/44456#discussion_r4064714280
##########
superset-frontend/src/pages/Home/Home.test.tsx:
##########
@@ -165,11 +171,35 @@ const renderWelcome = (props = mockedProps) =>
afterEach(() => {
fetchMock.clearHistory();
+ jest.mocked(redirect).mockClear();
});
+test.each([
+ ['anonymous', { roles: { Public: [] }, permissions: {}, groups: [] }],
+ ['guest', { ...mockedProps.user, userId: undefined }],
+ ['missing', undefined],
+])(
+ 'Redirects the %s user through the server without fetching Home data',
+ (_, user) => {
+ render(<Welcome user={user} />, { useRedux: true, useRouter: true });
+
+ expect(redirect).toHaveBeenCalledWith('/welcome/');
+ [
+ chartsEndpoint,
+ dashboardsEndpoint,
+ recentActivityEndpoint,
+ savedQueryEndpoint,
+ ].forEach(endpoint => {
+ expect(fetchMock.callHistory.calls(endpoint)).toHaveLength(0);
Review Comment:
The redirect happens in a `useEffect`, so asserting immediately after
`render()` can be timing-sensitive and lead to flaky tests depending on
React/RTL scheduling. Make the test async and wrap the redirect expectation
(and potentially the “no fetch calls” expectations) in `waitFor(...)` so it
deterministically waits for the effect to run.
##########
superset-frontend/src/pages/Home/index.tsx:
##########
@@ -448,4 +450,20 @@ function Welcome({ user, addDangerToast }: WelcomeProps) {
);
}
-export default withToasts(Welcome);
+function WelcomePage({
+ user,
+ ...props
+}: Omit<WelcomeProps, 'user'> & { user?: UserWithPermissionsAndRoles }) {
+ const hasUserId = Boolean(user?.userId);
Review Comment:
`Boolean(user?.userId)` treats `0` as “missing”, which can incorrectly
redirect if `userId` is ever `0` (or another falsy-but-valid value). Prefer an
explicit null/undefined check (e.g., `user?.userId != null`) so the guard
matches the intent: “ID is present”, not “truthy”.
##########
superset-frontend/src/pages/Home/Home.test.tsx:
##########
@@ -165,11 +171,35 @@ const renderWelcome = (props = mockedProps) =>
afterEach(() => {
fetchMock.clearHistory();
+ jest.mocked(redirect).mockClear();
});
+test.each([
+ ['anonymous', { roles: { Public: [] }, permissions: {}, groups: [] }],
+ ['guest', { ...mockedProps.user, userId: undefined }],
+ ['missing', undefined],
+])(
+ 'Redirects the %s user through the server without fetching Home data',
+ (_, user) => {
+ render(<Welcome user={user} />, { useRedux: true, useRouter: true });
+
+ expect(redirect).toHaveBeenCalledWith('/welcome/');
Review Comment:
The test hardcodes `'/welcome/'` while the production code uses a route
constant (`RoutePaths.*`). Import and assert against the same constant to avoid
tests drifting if the route path changes (especially relevant for deployments
with custom roots/paths).
##########
superset-frontend/src/pages/Home/index.tsx:
##########
@@ -448,4 +450,20 @@ function Welcome({ user, addDangerToast }: WelcomeProps) {
);
}
-export default withToasts(Welcome);
+function WelcomePage({
+ user,
+ ...props
+}: Omit<WelcomeProps, 'user'> & { user?: UserWithPermissionsAndRoles }) {
+ const hasUserId = Boolean(user?.userId);
+
+ useEffect(() => {
+ if (!hasUserId) {
+ // SPA navigation can bypass the welcome view's server-side login check.
+ redirect(RoutePaths.HOME);
+ }
+ }, [hasUserId]);
+
+ return user?.userId ? <Welcome {...props} user={user} /> : null;
Review Comment:
`hasUserId` is computed but the render path re-checks `user?.userId` instead
of using `hasUserId`. Using the same guard for both redirect and render makes
the logic easier to reason about and prevents divergence if the condition
changes later.
##########
superset-frontend/src/pages/Home/index.tsx:
##########
@@ -448,4 +450,20 @@ function Welcome({ user, addDangerToast }: WelcomeProps) {
);
}
-export default withToasts(Welcome);
+function WelcomePage({
+ user,
+ ...props
+}: Omit<WelcomeProps, 'user'> & { user?: UserWithPermissionsAndRoles }) {
+ const hasUserId = Boolean(user?.userId);
Review Comment:
`hasUserId` is computed but the render path re-checks `user?.userId` instead
of using `hasUserId`. Using the same guard for both redirect and render makes
the logic easier to reason about and prevents divergence if the condition
changes later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
