codeant-ai-for-open-source[bot] commented on code in PR #34640:
URL: https://github.com/apache/superset/pull/34640#discussion_r3464405637
##########
superset-frontend/src/pages/Login/index.tsx:
##########
@@ -79,7 +79,62 @@ const StyledLabel = styled(Typography.Text)`
`}
`;
+const StyledBackground = styled.div`
+ ${({ theme }) => {
+ const bgImageUrl = theme.loginPageBackgroundImageUrl;
+ const overlayColor =
+ theme.loginPageBackgroundOverlayColor || 'rgba(0, 0, 0, 0.5)';
+ const blurAmount = theme.loginPageBackgroundBlur || '0px';
+ return bgImageUrl
+ ? css`
+ position: fixed;
+ top: 0;
+ left: 0;
+ width: 100vw;
+ height: 100vh;
+ z-index: 0;
+ pointer-events: none;
+
+ &::before {
+ content: '';
+ position: absolute;
+ top: 0;
+ left: 0;
+ width: 100%;
+ height: 100%;
+ background-image: url(${bgImageUrl});
+ background-size: cover;
+ background-position: center;
+ filter: blur(${blurAmount});
+ z-index: -1;
+ }
+
+ &::after {
+ content: '';
+ position: absolute;
+ top: 0;
+ left: 0;
+ width: 100%;
+ height: 100%;
+ background-color: ${overlayColor};
+ z-index: 1;
+ }
Review Comment:
**Suggestion:** The background layer is given `z-index: 0` while the main
login container has no explicit stacking context, so the fixed overlay can
render above the login card instead of behind it. This causes the dark overlay
to tint the form itself (and can create visual/readability regressions). Move
the background behind content (for example with a negative z-index) or give the
login container/card a higher z-index with `position: relative`. [css layout
issue]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Themed login background darkens and obscures login form content.
- ⚠️ Visual regression in login page tests and branding screenshots.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a Superset theme that sets `loginPageBackgroundImageUrl` on the
Superset
theme tokens (see `loginPageBackgroundImageUrl?: string;` in
`superset-frontend/packages/superset-core/src/theme/types.ts:12-16`), so that
`theme.loginPageBackgroundImageUrl` used in `StyledBackground` at
`superset-frontend/src/pages/Login/index.tsx:83-88` is a non-empty string.
2. Start Superset with the PR frontend and navigate to the login page, e.g.
using the
Playwright helper `AuthPage.goto()` in
`superset-frontend/playwright/pages/AuthPage.ts:30-37`, which calls
`page.goto(URL.LOGIN)`
where `URL.LOGIN` is `'login/'`
(`superset-frontend/playwright/utils/urls.ts:30-36`),
causing the `Login` component in
`superset-frontend/src/pages/Login/index.tsx:135-362` to
render.
3. During render, `<StyledBackground />` at `index.tsx:212` creates a fixed
positioned
element with `z-index: 0` and `pointer-events: none` plus an absolutely
positioned
`::after` overlay with `background-color: overlayColor` at
`index.tsx:112-121`; the login
content is inside a separate `<Flex>` and `<StyledCard>` at
`index.tsx:213-248` with no
`position` or `z-index` specified.
4. According to CSS stacking rules, the positioned element with `z-index: 0`
(`StyledBackground`) is painted after non-positioned block elements (the
`<Flex>`
containing the card), so the dark overlay `::after` at `index.tsx:112-121`
visually
covers/tints the login card and its text, even though pointer events pass
through, causing
a noticeable readability regression whenever a background image is
configured.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=77119f7544f44715b926388d04e4264a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=77119f7544f44715b926388d04e4264a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/Login/index.tsx
**Line:** 90:121
**Comment:**
*Css Layout Issue: The background layer is given `z-index: 0` while the
main login container has no explicit stacking context, so the fixed overlay can
render above the login card instead of behind it. This causes the dark overlay
to tint the form itself (and can create visual/readability regressions). Move
the background behind content (for example with a negative z-index) or give the
login container/card a higher z-index with `position: relative`.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34640&comment_hash=b850c99568886cd64d57c47446e5f5cb3af8bcafc7b7384cc83bfc39971ac541&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34640&comment_hash=b850c99568886cd64d57c47446e5f5cb3af8bcafc7b7384cc83bfc39971ac541&reaction=dislike'>👎</a>
##########
superset-frontend/src/pages/Login/index.tsx:
##########
@@ -153,121 +208,156 @@ export default function Login() {
};
return (
- <Flex
- justify="center"
- align="center"
- data-test="login-form"
- css={css`
- width: 100%;
- height: calc(100vh - 200px);
- `}
- >
- <StyledCard title={t('Sign in')} padded>
- {authType === AuthType.AuthOID && (
- <Flex justify="center" vertical gap="middle">
- <Form layout="vertical" requiredMark="optional" form={form}>
- {providers.map((provider: OIDProvider) => (
- <Form.Item<LoginForm>>
- <Button
- href={buildProviderLoginUrl(provider.name)}
- block
- iconPosition="start"
- icon={getAuthIconElement(provider.name)}
- >
- {t('Sign in with')} {capitalize(provider.name)}
- </Button>
- </Form.Item>
- ))}
- </Form>
- </Flex>
- )}
- {(authType === AuthType.AuthOauth ||
- authType === AuthType.AuthSAML) && (
- <Flex justify="center" gap={0} vertical>
- <Form layout="vertical" requiredMark="optional" form={form}>
- {providers.map((provider: OAuthProvider) => (
- <Form.Item<LoginForm>>
- <Button
- href={buildProviderLoginUrl(provider.name)}
- block
- iconPosition="start"
- icon={getAuthIconElement(provider.name)}
- >
- {t('Sign in with')} {capitalize(provider.name)}
- </Button>
- </Form.Item>
- ))}
- </Form>
- </Flex>
- )}
-
- {(authType === AuthType.AuthDB || authType === AuthType.AuthLDAP) && (
- <Flex justify="center" vertical gap="middle">
- <Typography.Text type="secondary">
- {t('Enter your login and password below:')}
- </Typography.Text>
- <Form
- layout="vertical"
- requiredMark="optional"
- form={form}
- onFinish={onFinish}
- >
- <Form.Item<LoginForm>
- label={<StyledLabel>{t('Username:')}</StyledLabel>}
- name="username"
- rules={[
- { required: true, message: t('Please enter your username') },
- ]}
- >
- <Input
- autoFocus
- prefix={<Icons.UserOutlined iconSize="l" />}
- data-test="username-input"
+ <>
+ <StyledBackground />
+ <Flex
+ justify="center"
+ align="center"
+ data-test="login-form"
+ css={css`
+ width: 100%;
+ height: calc(100vh - 200px);
+ `}
+ >
+ <StyledCard
+ padded
+ title={
+ <Flex vertical align="start" gap="middle">
+ {theme.loginFormBrandLogoUrl && (
+ <StyledBrandLogo
+ src={theme.loginFormBrandLogoUrl}
+ alt={theme.brandLogoAlt || 'Apache Superset'}
+ style={{ marginTop: theme.sizeUnit * 4 }}
/>
- </Form.Item>
- <Form.Item<LoginForm>
- label={<StyledLabel>{t('Password:')}</StyledLabel>}
- name="password"
- rules={[
- { required: true, message: t('Please enter your password') },
- ]}
+ )}
+ <Typography.Title
+ level={4}
+ style={{
+ marginTop: 0,
+ marginBottom: theme.loginFormBrandLogoUrl
+ ? theme.sizeUnit * 2
+ : 0,
+ marginLeft: 0,
+ marginRight: 0,
+ }}
>
- <Input.Password
- prefix={<Icons.KeyOutlined iconSize="l" />}
- data-test="password-input"
- />
- </Form.Item>
- <Form.Item label={null}>
- <Flex
- css={css`
- width: 100%;
- `}
+ {t('Sign in')}
+ </Typography.Title>
+ </Flex>
+ }
+ >
+ {authType === AuthType.AuthOID && (
+ <Flex justify="center" vertical gap="middle">
+ <Form layout="vertical" requiredMark="optional" form={form}>
+ {providers.map((provider: OIDProvider) => (
+ <Form.Item<LoginForm>>
+ <Button
+ href={buildProviderLoginUrl(provider.name)}
+ block
+ iconPosition="start"
+ icon={getAuthIconElement(provider.name)}
+ >
+ {t('Sign in with')} {capitalize(provider.name)}
+ </Button>
+ </Form.Item>
+ ))}
Review Comment:
**Suggestion:** The list rendered from `providers.map(...)` does not assign
a React `key` to each repeated `Form.Item`, which breaks stable reconciliation
when provider lists change and triggers runtime warnings. Add a unique key (for
example provider name/URL) on each mapped item. [missing react key]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ React dev console shows key warnings on OIDC login page.
- ⚠️ Potential unstable reconciliation if OIDC providers list changes.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure the backend so that `bootstrapData.common.conf.AUTH_TYPE` equals
`AuthType.AuthOID` (enum defined at
`superset-frontend/src/pages/Login/index.tsx:55-61`)
and `bootstrapData.common.conf.AUTH_PROVIDERS` contains one or more OIDC
providers (read
into `providers: Provider[]` at `index.tsx:164-165`).
2. Start Superset with the PR frontend and navigate to the login page using
`AuthPage.goto()` (`superset-frontend/playwright/pages/AuthPage.ts:30-37`),
which loads
the `Login` component defined in
`superset-frontend/src/pages/Login/index.tsx:135-362`.
3. With `authType === AuthType.AuthOID`, the branch at `index.tsx:249-266`
executes and
renders `{providers.map((provider: OIDProvider) => ( <Form.Item<LoginForm>>
...
</Form.Item> ))}` at `index.tsx:252-263` without passing a `key` prop to
`<Form.Item>`.
4. In React development mode, this mapped list without keys triggers the
standard runtime
warning "Each child in a list should have a unique 'key' prop" in the
browser console on
every render of the OIDC login provider list, and if the `providers` array
were to change
or reorder between renders, reconciliation would be based on index instead
of provider
identity.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=35807b0a3e594beaa3ae68f67fb81f47&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=35807b0a3e594beaa3ae68f67fb81f47&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/Login/index.tsx
**Line:** 252:263
**Comment:**
*Missing React Key: The list rendered from `providers.map(...)` does
not assign a React `key` to each repeated `Form.Item`, which breaks stable
reconciliation when provider lists change and triggers runtime warnings. Add a
unique key (for example provider name/URL) on each mapped item.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34640&comment_hash=dceb186203a3bed69aeff0ed3c64e2e78b7a789df2fb2460785ae7b3b08bd9d9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34640&comment_hash=dceb186203a3bed69aeff0ed3c64e2e78b7a789df2fb2460785ae7b3b08bd9d9&reaction=dislike'>👎</a>
##########
superset-frontend/src/pages/Login/index.tsx:
##########
@@ -153,121 +208,156 @@ export default function Login() {
};
return (
- <Flex
- justify="center"
- align="center"
- data-test="login-form"
- css={css`
- width: 100%;
- height: calc(100vh - 200px);
- `}
- >
- <StyledCard title={t('Sign in')} padded>
- {authType === AuthType.AuthOID && (
- <Flex justify="center" vertical gap="middle">
- <Form layout="vertical" requiredMark="optional" form={form}>
- {providers.map((provider: OIDProvider) => (
- <Form.Item<LoginForm>>
- <Button
- href={buildProviderLoginUrl(provider.name)}
- block
- iconPosition="start"
- icon={getAuthIconElement(provider.name)}
- >
- {t('Sign in with')} {capitalize(provider.name)}
- </Button>
- </Form.Item>
- ))}
- </Form>
- </Flex>
- )}
- {(authType === AuthType.AuthOauth ||
- authType === AuthType.AuthSAML) && (
- <Flex justify="center" gap={0} vertical>
- <Form layout="vertical" requiredMark="optional" form={form}>
- {providers.map((provider: OAuthProvider) => (
- <Form.Item<LoginForm>>
- <Button
- href={buildProviderLoginUrl(provider.name)}
- block
- iconPosition="start"
- icon={getAuthIconElement(provider.name)}
- >
- {t('Sign in with')} {capitalize(provider.name)}
- </Button>
- </Form.Item>
- ))}
- </Form>
- </Flex>
- )}
-
- {(authType === AuthType.AuthDB || authType === AuthType.AuthLDAP) && (
- <Flex justify="center" vertical gap="middle">
- <Typography.Text type="secondary">
- {t('Enter your login and password below:')}
- </Typography.Text>
- <Form
- layout="vertical"
- requiredMark="optional"
- form={form}
- onFinish={onFinish}
- >
- <Form.Item<LoginForm>
- label={<StyledLabel>{t('Username:')}</StyledLabel>}
- name="username"
- rules={[
- { required: true, message: t('Please enter your username') },
- ]}
- >
- <Input
- autoFocus
- prefix={<Icons.UserOutlined iconSize="l" />}
- data-test="username-input"
+ <>
+ <StyledBackground />
+ <Flex
+ justify="center"
+ align="center"
+ data-test="login-form"
+ css={css`
+ width: 100%;
+ height: calc(100vh - 200px);
+ `}
+ >
+ <StyledCard
+ padded
+ title={
+ <Flex vertical align="start" gap="middle">
+ {theme.loginFormBrandLogoUrl && (
+ <StyledBrandLogo
+ src={theme.loginFormBrandLogoUrl}
+ alt={theme.brandLogoAlt || 'Apache Superset'}
+ style={{ marginTop: theme.sizeUnit * 4 }}
/>
- </Form.Item>
- <Form.Item<LoginForm>
- label={<StyledLabel>{t('Password:')}</StyledLabel>}
- name="password"
- rules={[
- { required: true, message: t('Please enter your password') },
- ]}
+ )}
+ <Typography.Title
+ level={4}
+ style={{
+ marginTop: 0,
+ marginBottom: theme.loginFormBrandLogoUrl
+ ? theme.sizeUnit * 2
+ : 0,
+ marginLeft: 0,
+ marginRight: 0,
+ }}
>
- <Input.Password
- prefix={<Icons.KeyOutlined iconSize="l" />}
- data-test="password-input"
- />
- </Form.Item>
- <Form.Item label={null}>
- <Flex
- css={css`
- width: 100%;
- `}
+ {t('Sign in')}
+ </Typography.Title>
+ </Flex>
+ }
+ >
+ {authType === AuthType.AuthOID && (
+ <Flex justify="center" vertical gap="middle">
+ <Form layout="vertical" requiredMark="optional" form={form}>
+ {providers.map((provider: OIDProvider) => (
+ <Form.Item<LoginForm>>
+ <Button
+ href={buildProviderLoginUrl(provider.name)}
+ block
+ iconPosition="start"
+ icon={getAuthIconElement(provider.name)}
+ >
+ {t('Sign in with')} {capitalize(provider.name)}
+ </Button>
+ </Form.Item>
+ ))}
+ </Form>
+ </Flex>
+ )}
+ {(authType === AuthType.AuthOauth ||
+ authType === AuthType.AuthSAML) && (
+ <Flex justify="center" gap={0} vertical>
+ <Form layout="vertical" requiredMark="optional" form={form}>
+ {providers.map((provider: OAuthProvider) => (
+ <Form.Item<LoginForm>>
+ <Button
+ href={buildProviderLoginUrl(provider.name)}
+ block
+ iconPosition="start"
+ icon={getAuthIconElement(provider.name)}
+ >
+ {t('Sign in with')} {capitalize(provider.name)}
+ </Button>
+ </Form.Item>
+ ))}
Review Comment:
**Suggestion:** The OAuth/SAML provider list is also missing a `key` per
mapped `Form.Item`, creating the same reconciliation instability and warning
noise in this branch as well. Add a deterministic unique key for each provider
row. [missing react key]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ React dev console shows key warnings for OAuth/SAML providers.
- ⚠️ Possible unstable diffing when OAuth/SAML providers configuration
changes.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure the backend so that `bootstrapData.common.conf.AUTH_TYPE`
equals either
`AuthType.AuthOauth` or `AuthType.AuthSAML` (enum at
`superset-frontend/src/pages/Login/index.tsx:55-61`) and
`bootstrapData.common.conf.AUTH_PROVIDERS` contains one or more OAuth/SAML
providers
(consumed into `providers: Provider[]` at `index.tsx:164-165`).
2. Run Superset with this PR and open the login page via `AuthPage.goto()`
(`superset-frontend/playwright/pages/AuthPage.ts:30-37`, which navigates to
`URL.LOGIN`
defined as `'login/'` in
`superset-frontend/playwright/utils/urls.ts:30-36`), causing the
`Login` component in `superset-frontend/src/pages/Login/index.tsx:135-362`
to render.
3. When `authType` is `AuthType.AuthOauth` or `AuthType.AuthSAML`, the
branch at
`index.tsx:267-285` renders `{providers.map((provider: OAuthProvider) => (
<Form.Item<LoginForm>> ... </Form.Item> ))}` at `index.tsx:271-282` with no
`key` prop on
`<Form.Item>`.
4. As with the OIDC branch, React development mode logs the "Each child in a
list should
have a unique 'key' prop" warning for this OAuth/SAML provider list on each
render, and
any runtime changes to the providers array would be reconciled by index
rather than by
stable provider identity.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=dc43eaff33424b5cbf556bd0232d5903&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=dc43eaff33424b5cbf556bd0232d5903&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/Login/index.tsx
**Line:** 271:282
**Comment:**
*Missing React Key: The OAuth/SAML provider list is also missing a
`key` per mapped `Form.Item`, creating the same reconciliation instability and
warning noise in this branch as well. Add a deterministic unique key for each
provider row.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34640&comment_hash=f507da4044283b8600e8c5ff349204f802e4cd7a5f80117b8a12bf024f38b532&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34640&comment_hash=f507da4044283b8600e8c5ff349204f802e4cd7a5f80117b8a12bf024f38b532&reaction=dislike'>👎</a>
--
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]
