korbit-ai[bot] commented on code in PR #33255:
URL: https://github.com/apache/superset/pull/33255#discussion_r2097571613
##########
superset/views/alerts.py:
##########
@@ -35,6 +34,8 @@ class BaseAlertReportView(BaseSupersetView):
@has_access
@permission_name("read")
def list(self) -> FlaskResponse:
+ from superset import is_feature_enabled
Review Comment:
### Potential Circular Import Risk <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The feature check import has been moved inside the methods, which could
cause circular imports if other modules depend on this module and also import
is_feature_enabled.
###### Why this matters
Circular imports can lead to runtime errors and make the application fail to
start. This is particularly critical for a login-related feature where a
failure could prevent users from accessing the system.
###### Suggested change ∙ *Feature Preview*
Move the import back to the top of the file and use lazy loading only if
absolutely necessary to resolve circular dependencies:
```python
from superset import is_feature_enabled # At the top of the file
# Remove the imports from inside the methods
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3c7b20af-2773-4ca7-b476-db26ad64c3c2/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3c7b20af-2773-4ca7-b476-db26ad64c3c2?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3c7b20af-2773-4ca7-b476-db26ad64c3c2?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3c7b20af-2773-4ca7-b476-db26ad64c3c2?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3c7b20af-2773-4ca7-b476-db26ad64c3c2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:e40b9a47-8fe0-4496-8d4b-803175c731ae -->
[](e40b9a47-8fe0-4496-8d4b-803175c731ae)
##########
superset-frontend/src/pages/Register/index.tsx:
##########
@@ -0,0 +1,209 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { SupersetClient, styled, t, css } from '@superset-ui/core';
+import { Button, Card, Flex, Form, Input } from 'src/components';
+import { useState } from 'react';
+import getBootstrapData from 'src/utils/getBootstrapData';
+import ReactCAPTCHA from 'react-google-recaptcha';
+
+interface RegisterForm {
+ username: string;
+ firstName: string;
+ lastName: string;
+ email: string;
+ password: string;
+ confirmPassword: string;
+}
+
+const StyledCard = styled(Card)`
+ ${({ theme }) => css`
+ width: 50%;
+ margin-top: ${theme.marginXL}px;
+ background: ${theme.colorBgBase};
+ .antd5-form-item-label label {
+ color: ${theme.colorPrimary};
+ }
+ `}
+`;
+
+const formItemLayout = {
+ labelCol: {
+ xs: { span: 24 },
+ sm: { span: 6 },
+ },
+ wrapperCol: {
+ xs: { span: 24 },
+ sm: { span: 24 },
+ },
+};
+
+export default function Login() {
+ const [form] = Form.useForm<RegisterForm>();
+ const [loading, setLoading] = useState(false);
+ const [captchaResponse, setCaptchaResponse] = useState<string | null>(null);
+
+ const bootstrapData = getBootstrapData();
+
+ const authRecaptchaPublicKey: string =
+ bootstrapData.common.conf.RECAPTCHA_PUBLIC_KEY;
+
+ const onFinish = (values: RegisterForm) => {
+ setLoading(true);
+ const payload = {
+ username: values.username,
+ first_name: values.firstName,
+ last_name: values.lastName,
+ email: values.email,
+ password: values.password,
+ conf_password: values.confirmPassword,
+ 'g-recaptcha-response': captchaResponse,
+ };
+ SupersetClient.postForm('/register/form', payload, '').finally(() => {
Review Comment:
### Unclear API Parameter <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The empty string parameter in the postForm call is not self-explanatory and
its purpose is unclear.
###### Why this matters
Developers need to check the SupersetClient documentation to understand why
an empty string is passed, reducing code clarity.
###### Suggested change ∙ *Feature Preview*
```typescript
const EMPTY_FORM_ENCODING = ''; // Default form encoding
SupersetClient.postForm('/register/form', payload,
EMPTY_FORM_ENCODING).finally(() => {
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f536666b-905f-4e82-bfd3-15e507054bd0/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f536666b-905f-4e82-bfd3-15e507054bd0?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f536666b-905f-4e82-bfd3-15e507054bd0?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f536666b-905f-4e82-bfd3-15e507054bd0?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f536666b-905f-4e82-bfd3-15e507054bd0)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:add37a0e-984a-457b-a06c-be937a825706 -->
[](add37a0e-984a-457b-a06c-be937a825706)
##########
superset-frontend/src/pages/Register/index.tsx:
##########
@@ -0,0 +1,209 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { SupersetClient, styled, t, css } from '@superset-ui/core';
+import { Button, Card, Flex, Form, Input } from 'src/components';
+import { useState } from 'react';
+import getBootstrapData from 'src/utils/getBootstrapData';
+import ReactCAPTCHA from 'react-google-recaptcha';
+
+interface RegisterForm {
+ username: string;
+ firstName: string;
+ lastName: string;
+ email: string;
+ password: string;
+ confirmPassword: string;
+}
Review Comment:
### Missing Interface Documentation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The RegisterForm interface lacks documentation about its purpose and field
requirements.
###### Why this matters
Without clear documentation about validation rules and field requirements,
developers might make incorrect assumptions about the form's behavior.
###### Suggested change ∙ *Feature Preview*
/**
* Interface representing the registration form data structure.
* All fields are required and validated according to registration rules.
*/
interface RegisterForm {
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2ca204d5-e25a-4aeb-a05c-5762ed14689d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2ca204d5-e25a-4aeb-a05c-5762ed14689d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2ca204d5-e25a-4aeb-a05c-5762ed14689d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2ca204d5-e25a-4aeb-a05c-5762ed14689d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2ca204d5-e25a-4aeb-a05c-5762ed14689d)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:b4de068e-fcf8-478b-81d4-39e70dfbe209 -->
[](b4de068e-fcf8-478b-81d4-39e70dfbe209)
##########
superset-frontend/src/pages/Register/index.tsx:
##########
@@ -0,0 +1,209 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { SupersetClient, styled, t, css } from '@superset-ui/core';
+import { Button, Card, Flex, Form, Input } from 'src/components';
+import { useState } from 'react';
+import getBootstrapData from 'src/utils/getBootstrapData';
+import ReactCAPTCHA from 'react-google-recaptcha';
+
+interface RegisterForm {
+ username: string;
+ firstName: string;
+ lastName: string;
+ email: string;
+ password: string;
+ confirmPassword: string;
+}
+
+const StyledCard = styled(Card)`
+ ${({ theme }) => css`
+ width: 50%;
+ margin-top: ${theme.marginXL}px;
+ background: ${theme.colorBgBase};
+ .antd5-form-item-label label {
+ color: ${theme.colorPrimary};
+ }
+ `}
+`;
+
+const formItemLayout = {
+ labelCol: {
+ xs: { span: 24 },
+ sm: { span: 6 },
+ },
+ wrapperCol: {
+ xs: { span: 24 },
+ sm: { span: 24 },
+ },
+};
+
+export default function Login() {
+ const [form] = Form.useForm<RegisterForm>();
+ const [loading, setLoading] = useState(false);
+ const [captchaResponse, setCaptchaResponse] = useState<string | null>(null);
+
+ const bootstrapData = getBootstrapData();
+
+ const authRecaptchaPublicKey: string =
+ bootstrapData.common.conf.RECAPTCHA_PUBLIC_KEY;
+
+ const onFinish = (values: RegisterForm) => {
+ setLoading(true);
+ const payload = {
+ username: values.username,
+ first_name: values.firstName,
+ last_name: values.lastName,
+ email: values.email,
+ password: values.password,
+ conf_password: values.confirmPassword,
+ 'g-recaptcha-response': captchaResponse,
+ };
+ SupersetClient.postForm('/register/form', payload, '').finally(() => {
+ setLoading(false);
+ });
Review Comment:
### Form Submission Logic Not Separated <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The form submission logic is embedded within the component and handles API
calls directly, violating the Single Responsibility Principle.
###### Why this matters
Direct API calls in components make them harder to test, maintain, and
reuse. They also make it difficult to handle global error states and loading
states consistently.
###### Suggested change ∙ *Feature Preview*
Extract the registration logic into a separate service or custom hook:
```typescript
// hooks/useRegister.ts
export const useRegister = () => {
const register = async (formData: RegisterForm) => {
// registration logic here
};
return { register };
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cbbf0ff6-7ea6-4301-a7b4-4228f9113f4d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cbbf0ff6-7ea6-4301-a7b4-4228f9113f4d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cbbf0ff6-7ea6-4301-a7b4-4228f9113f4d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cbbf0ff6-7ea6-4301-a7b4-4228f9113f4d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cbbf0ff6-7ea6-4301-a7b4-4228f9113f4d)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:cebc920e-f2ee-4f8f-acce-4d96090f5325 -->
[](cebc920e-f2ee-4f8f-acce-4d96090f5325)
##########
superset-frontend/src/views/routes.tsx:
##########
@@ -146,6 +154,18 @@ type Routes = {
}[];
export const routes: Routes = [
+ {
+ path: '/login/',
+ Component: Login,
+ },
+ {
+ path: '/register/',
+ Component: Register,
+ },
+ {
+ path: '/logout/',
+ Component: Login,
+ },
Review Comment:
### Incorrect Component for Logout Route <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The logout route is incorrectly using the Login component, which doesn't
handle the logout functionality.
###### Why this matters
Users attempting to logout will be shown the login page without their
session being terminated, leading to an inconsistent authentication state.
###### Suggested change ∙ *Feature Preview*
Create and use a dedicated Logout component that handles session termination
before redirecting to the login page. Example:
```typescript
const Logout = lazy(
() => import(/* webpackChunkName: "Logout" */ 'src/pages/Logout'),
);
// In routes array
{
path: '/logout/',
Component: Logout,
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0a772a37-67aa-48a6-8d94-016823b71823/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0a772a37-67aa-48a6-8d94-016823b71823?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0a772a37-67aa-48a6-8d94-016823b71823?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0a772a37-67aa-48a6-8d94-016823b71823?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0a772a37-67aa-48a6-8d94-016823b71823)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:933641a1-f00d-4d3f-939a-47f2f471390e -->
[](933641a1-f00d-4d3f-939a-47f2f471390e)
##########
superset/views/auth.py:
##########
@@ -0,0 +1,46 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from typing import Optional
+
+from flask import g, redirect
+from flask_appbuilder import expose
+from flask_appbuilder.security.decorators import no_cache
+from flask_appbuilder.security.views import AuthView, WerkzeugResponse
+
+from superset.views.base import BaseSupersetView
+
+
+class SupersetAuthView(BaseSupersetView, AuthView):
+ route_base = "/login"
+
+ @expose("/")
+ @no_cache
+ def login(self, provider: Optional[str] = None) -> WerkzeugResponse:
Review Comment:
### Unused OAuth Provider Parameter <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The 'provider' parameter is defined but never used in the login method,
despite the intent to support OAuth providers.
###### Why this matters
OAuth provider selection will not work as the parameter is not being
handled, preventing users from logging in through social providers like Google,
Github, and Facebook.
###### Suggested change ∙ *Feature Preview*
Implement provider handling in the login method:
```python
def login(self, provider: Optional[str] = None) -> WerkzeugResponse:
if g.user is not None and g.user.is_authenticated:
return redirect(self.appbuilder.get_url_for_index)
if provider:
return self.oauth_login(provider)
return super().render_app_template()
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c14ed5f3-97e6-4639-8305-fce387397cd2/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c14ed5f3-97e6-4639-8305-fce387397cd2?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c14ed5f3-97e6-4639-8305-fce387397cd2?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c14ed5f3-97e6-4639-8305-fce387397cd2?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c14ed5f3-97e6-4639-8305-fce387397cd2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d3c3b280-de0c-4cd9-9347-3d15ca317eea -->
[](d3c3b280-de0c-4cd9-9347-3d15ca317eea)
##########
superset/views/auth.py:
##########
@@ -0,0 +1,46 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from typing import Optional
+
+from flask import g, redirect
+from flask_appbuilder import expose
+from flask_appbuilder.security.decorators import no_cache
+from flask_appbuilder.security.views import AuthView, WerkzeugResponse
+
+from superset.views.base import BaseSupersetView
+
+
+class SupersetAuthView(BaseSupersetView, AuthView):
+ route_base = "/login"
+
+ @expose("/")
+ @no_cache
+ def login(self, provider: Optional[str] = None) -> WerkzeugResponse:
+ if g.user is not None and g.user.is_authenticated:
+ return redirect(self.appbuilder.get_url_for_index)
Review Comment:
### Unhandled URL Generation Error in Redirect <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The redirect call using get_url_for_index may raise an exception if there's
an issue with URL generation, but it's not handled.
###### Why this matters
If URL generation fails (e.g., due to misconfiguration or missing endpoint),
an unhandled exception would result in a 500 error instead of gracefully
falling back to a default URL.
###### Suggested change ∙ *Feature Preview*
Add try-except block to handle URL generation errors:
```python
try:
return redirect(self.appbuilder.get_url_for_index)
except Exception as e:
logger.error(f"Failed to generate index URL: {e}")
return redirect("/")
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d277daf8-094f-40ab-8640-9f36613ca204/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d277daf8-094f-40ab-8640-9f36613ca204?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d277daf8-094f-40ab-8640-9f36613ca204?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d277daf8-094f-40ab-8640-9f36613ca204?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d277daf8-094f-40ab-8640-9f36613ca204)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:5834c13c-d713-4f61-b7a8-f4e99d0d80c6 -->
[](5834c13c-d713-4f61-b7a8-f4e99d0d80c6)
##########
superset-frontend/src/pages/Register/index.tsx:
##########
@@ -0,0 +1,209 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { SupersetClient, styled, t, css } from '@superset-ui/core';
+import { Button, Card, Flex, Form, Input } from 'src/components';
+import { useState } from 'react';
+import getBootstrapData from 'src/utils/getBootstrapData';
+import ReactCAPTCHA from 'react-google-recaptcha';
+
+interface RegisterForm {
+ username: string;
+ firstName: string;
+ lastName: string;
+ email: string;
+ password: string;
+ confirmPassword: string;
+}
+
+const StyledCard = styled(Card)`
+ ${({ theme }) => css`
+ width: 50%;
+ margin-top: ${theme.marginXL}px;
+ background: ${theme.colorBgBase};
+ .antd5-form-item-label label {
+ color: ${theme.colorPrimary};
+ }
+ `}
+`;
+
+const formItemLayout = {
+ labelCol: {
+ xs: { span: 24 },
+ sm: { span: 6 },
+ },
+ wrapperCol: {
+ xs: { span: 24 },
+ sm: { span: 24 },
+ },
+};
+
+export default function Login() {
+ const [form] = Form.useForm<RegisterForm>();
+ const [loading, setLoading] = useState(false);
+ const [captchaResponse, setCaptchaResponse] = useState<string | null>(null);
+
+ const bootstrapData = getBootstrapData();
+
+ const authRecaptchaPublicKey: string =
+ bootstrapData.common.conf.RECAPTCHA_PUBLIC_KEY;
+
+ const onFinish = (values: RegisterForm) => {
+ setLoading(true);
+ const payload = {
+ username: values.username,
+ first_name: values.firstName,
+ last_name: values.lastName,
+ email: values.email,
+ password: values.password,
+ conf_password: values.confirmPassword,
+ 'g-recaptcha-response': captchaResponse,
+ };
Review Comment:
### Unmemoized Form Handler <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The form submission handler is recreated on every render due to not being
memoized.
###### Why this matters
Recreation of the function on every render can lead to unnecessary
re-renders of child components that receive this function as a prop.
###### Suggested change ∙ *Feature Preview*
Use useCallback to memoize the form submission handler:
```typescript
const onFinish = useCallback((values: RegisterForm) => {
setLoading(true);
const payload = {
username: values.username,
first_name: values.firstName,
last_name: values.lastName,
email: values.email,
password: values.password,
conf_password: values.confirmPassword,
'g-recaptcha-response': captchaResponse,
};
SupersetClient.postForm('/register/form', payload, '').finally(() => {
setLoading(false);
});
}, [captchaResponse]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d0ae058d-7022-400a-b027-51f5cc98be3e/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d0ae058d-7022-400a-b027-51f5cc98be3e?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d0ae058d-7022-400a-b027-51f5cc98be3e?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d0ae058d-7022-400a-b027-51f5cc98be3e?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d0ae058d-7022-400a-b027-51f5cc98be3e)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:70d2db9a-e923-48a5-8ae6-b6d6d51d1c8c -->
[](70d2db9a-e923-48a5-8ae6-b6d6d51d1c8c)
##########
superset-frontend/src/pages/Register/index.tsx:
##########
@@ -0,0 +1,209 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { SupersetClient, styled, t, css } from '@superset-ui/core';
+import { Button, Card, Flex, Form, Input } from 'src/components';
+import { useState } from 'react';
+import getBootstrapData from 'src/utils/getBootstrapData';
+import ReactCAPTCHA from 'react-google-recaptcha';
+
+interface RegisterForm {
+ username: string;
+ firstName: string;
+ lastName: string;
+ email: string;
+ password: string;
+ confirmPassword: string;
+}
+
+const StyledCard = styled(Card)`
+ ${({ theme }) => css`
+ width: 50%;
+ margin-top: ${theme.marginXL}px;
+ background: ${theme.colorBgBase};
+ .antd5-form-item-label label {
+ color: ${theme.colorPrimary};
+ }
+ `}
+`;
+
+const formItemLayout = {
+ labelCol: {
+ xs: { span: 24 },
+ sm: { span: 6 },
+ },
+ wrapperCol: {
+ xs: { span: 24 },
+ sm: { span: 24 },
+ },
+};
Review Comment:
### Unexplained Layout Constants <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The formItemLayout object contains unexplained magic numbers for responsive
layout spans.
###### Why this matters
Without context, developers need to mentally parse what these numbers mean
for the layout, making the code harder to maintain.
###### Suggested change ∙ *Feature Preview*
```typescript
// Layout constants for form responsiveness
const FULL_WIDTH_SPAN = 24;
const LABEL_DESKTOP_SPAN = 6;
const formItemLayout = {
labelCol: {
xs: { span: FULL_WIDTH_SPAN }, // Full width on extra small screens
sm: { span: LABEL_DESKTOP_SPAN }, // Fixed width on larger screens
},
wrapperCol: {
xs: { span: FULL_WIDTH_SPAN }, // Full width on extra small screens
sm: { span: FULL_WIDTH_SPAN }, // Full width on larger screens
},
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/17caf8cd-2757-45b0-b63d-f3e0cbc4734f/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/17caf8cd-2757-45b0-b63d-f3e0cbc4734f?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/17caf8cd-2757-45b0-b63d-f3e0cbc4734f?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/17caf8cd-2757-45b0-b63d-f3e0cbc4734f?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/17caf8cd-2757-45b0-b63d-f3e0cbc4734f)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:7038e811-13a7-4267-ac7b-e94efa1460fa -->
[](7038e811-13a7-4267-ac7b-e94efa1460fa)
##########
superset-frontend/src/pages/Register/index.tsx:
##########
@@ -0,0 +1,209 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { SupersetClient, styled, t, css } from '@superset-ui/core';
+import { Button, Card, Flex, Form, Input } from 'src/components';
+import { useState } from 'react';
+import getBootstrapData from 'src/utils/getBootstrapData';
+import ReactCAPTCHA from 'react-google-recaptcha';
+
+interface RegisterForm {
+ username: string;
+ firstName: string;
+ lastName: string;
+ email: string;
+ password: string;
+ confirmPassword: string;
+}
+
+const StyledCard = styled(Card)`
+ ${({ theme }) => css`
+ width: 50%;
+ margin-top: ${theme.marginXL}px;
+ background: ${theme.colorBgBase};
+ .antd5-form-item-label label {
+ color: ${theme.colorPrimary};
+ }
+ `}
+`;
+
+const formItemLayout = {
+ labelCol: {
+ xs: { span: 24 },
+ sm: { span: 6 },
+ },
+ wrapperCol: {
+ xs: { span: 24 },
+ sm: { span: 24 },
+ },
+};
+
+export default function Login() {
+ const [form] = Form.useForm<RegisterForm>();
+ const [loading, setLoading] = useState(false);
+ const [captchaResponse, setCaptchaResponse] = useState<string | null>(null);
+
+ const bootstrapData = getBootstrapData();
Review Comment:
### Unmemoized Bootstrap Data Fetch <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The bootstrap data is being fetched on every render since it's not memoized.
###### Why this matters
Unnecessary re-fetching of bootstrap data on each render can impact
performance, especially if the data is large or the component re-renders
frequently.
###### Suggested change ∙ *Feature Preview*
Use useMemo to cache the bootstrap data:
```typescript
const bootstrapData = useMemo(() => getBootstrapData(), []);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ba2ab344-21c7-42cb-91a4-ceeb028f8730/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ba2ab344-21c7-42cb-91a4-ceeb028f8730?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ba2ab344-21c7-42cb-91a4-ceeb028f8730?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ba2ab344-21c7-42cb-91a4-ceeb028f8730?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ba2ab344-21c7-42cb-91a4-ceeb028f8730)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d9ef0e36-52f0-4275-98f4-873c5b907ec9 -->
[](d9ef0e36-52f0-4275-98f4-873c5b907ec9)
--
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]
