korbit-ai[bot] commented on code in PR #33301:
URL: https://github.com/apache/superset/pull/33301#discussion_r2115725271
##########
superset-frontend/src/features/roles/RoleFormItems.tsx:
##########
@@ -58,13 +66,28 @@ export const PermissionsField: FC<PermissionsFieldProps> =
({
</FormItem>
);
-export const UsersField: FC<UsersFieldProps> = ({ users }) => (
+export const UsersField = ({ addDangerToast, loading }: UsersFieldProps) => (
<FormItem name="roleUsers" label={t('Users')}>
+ <AsyncSelect
+ name="roleUsers"
+ mode="multiple"
+ placeholder={t('Select users')}
+ options={(filterValue, page, pageSize) =>
+ fetchUserOptions(filterValue, page, pageSize, addDangerToast)
+ }
Review Comment:
### Missing parameter type annotations <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Inline arrow function with multiple parameters could be more readable with
type annotations
###### Why this matters
Missing type annotations on callback parameters make it harder to understand
the expected input types without checking the implementation
###### Suggested change ∙ *Feature Preview*
```typescript
options={(filterValue: string, page: number, pageSize: number) =>
fetchUserOptions(filterValue, page, pageSize, addDangerToast)
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/caab81fa-b1d9-4d1b-b23e-c309469ecb29/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/caab81fa-b1d9-4d1b-b23e-c309469ecb29?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/caab81fa-b1d9-4d1b-b23e-c309469ecb29?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/caab81fa-b1d9-4d1b-b23e-c309469ecb29?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/caab81fa-b1d9-4d1b-b23e-c309469ecb29)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:07ec26e0-a5f0-4bf3-b65e-916c854e457f -->
[](07ec26e0-a5f0-4bf3-b65e-916c854e457f)
##########
superset-frontend/src/features/roles/utils.ts:
##########
@@ -40,6 +40,12 @@ export const updateRoleUsers = async (roleId: number,
userIds: number[]) =>
jsonPayload: { user_ids: userIds },
});
+export const updateRoleGroups = async (roleId: number, groupIds: number[]) =>
+ SupersetClient.put({
+ endpoint: `/api/v1/security/roles/${roleId}/groups`,
+ jsonPayload: { group_ids: groupIds },
+ });
Review Comment:
### Missing Error Handling in Role Groups Update <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The function is missing error handling for failed API requests, which is
inconsistent with typical REST API interaction patterns.
###### Why this matters
Without error handling, failed group updates will silently fail, making it
difficult for the UI to inform users of problems and potentially leaving the
system in an inconsistent state.
###### Suggested change ∙ *Feature Preview*
Add try-catch block and return type annotation for better error handling:
```typescript
export const updateRoleGroups = async (
roleId: number,
groupIds: number[],
): Promise<Response | void> => {
try {
return await SupersetClient.put({
endpoint: `/api/v1/security/roles/${roleId}/groups`,
jsonPayload: { group_ids: groupIds },
});
} catch (error) {
throw new Error(`Failed to update role groups: ${error}`);
}
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/86dc9055-11f7-4a00-92b6-284abd9c232f/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/86dc9055-11f7-4a00-92b6-284abd9c232f?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/86dc9055-11f7-4a00-92b6-284abd9c232f?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/86dc9055-11f7-4a00-92b6-284abd9c232f?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/86dc9055-11f7-4a00-92b6-284abd9c232f)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:1e519feb-09ad-4ea6-9fb1-1775980fcd89 -->
[](1e519feb-09ad-4ea6-9fb1-1775980fcd89)
##########
superset-frontend/src/features/roles/RoleFormItems.tsx:
##########
@@ -58,13 +66,28 @@
</FormItem>
);
-export const UsersField: FC<UsersFieldProps> = ({ users }) => (
+export const UsersField = ({ addDangerToast, loading }: UsersFieldProps) => (
<FormItem name="roleUsers" label={t('Users')}>
+ <AsyncSelect
+ name="roleUsers"
+ mode="multiple"
+ placeholder={t('Select users')}
+ options={(filterValue, page, pageSize) =>
+ fetchUserOptions(filterValue, page, pageSize, addDangerToast)
Review Comment:
### Insufficient Error Context in Async Callback <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The error handling via addDangerToast is hidden within a callback and lacks
contextual information about the error source
###### Why this matters
When an error occurs during user option fetching, the error message may not
provide sufficient context about where the error originated, making debugging
more difficult
###### Suggested change ∙ *Feature Preview*
Modify the fetchUserOptions call to include the operation context in the
error message:
```typescript
options={(filterValue, page, pageSize) =>
fetchUserOptions(filterValue, page, pageSize,
(error) => addDangerToast(`Failed to fetch user options: ${error}`)
)
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3d481c3b-6341-489b-baf2-3ebee0e90f2a/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3d481c3b-6341-489b-baf2-3ebee0e90f2a?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3d481c3b-6341-489b-baf2-3ebee0e90f2a?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3d481c3b-6341-489b-baf2-3ebee0e90f2a?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3d481c3b-6341-489b-baf2-3ebee0e90f2a)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:8375e6a7-0cec-4b6a-b6b4-fad3bb8b6b8b -->
[](8375e6a7-0cec-4b6a-b6b4-fad3bb8b6b8b)
##########
superset-frontend/src/features/roles/RoleFormItems.tsx:
##########
@@ -58,13 +66,28 @@
</FormItem>
);
-export const UsersField: FC<UsersFieldProps> = ({ users }) => (
+export const UsersField = ({ addDangerToast, loading }: UsersFieldProps) => (
<FormItem name="roleUsers" label={t('Users')}>
+ <AsyncSelect
+ name="roleUsers"
+ mode="multiple"
+ placeholder={t('Select users')}
+ options={(filterValue, page, pageSize) =>
+ fetchUserOptions(filterValue, page, pageSize, addDangerToast)
+ }
+ loading={loading}
+ data-test="roles-select"
+ />
+ </FormItem>
+);
+
+export const GroupsField: FC<GroupsFieldProps> = ({ groups }) => (
Review Comment:
### Missing error handling in GroupsField <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The GroupsField component lacks error handling for the scenario where the
groups array might be empty or undefined.
###### Why this matters
Without proper error handling, the component might render incorrectly or
cause runtime errors when the groups data is not available.
###### Suggested change ∙ *Feature Preview*
Add error handling and default value for groups:
```typescript
export const GroupsField: FC<GroupsFieldProps> = ({ groups = [] }) => (
<FormItem name="roleGroups" label={t('Groups')}>
<Select
mode="multiple"
name="roleGroups"
options={(groups || []).map(group => ({ label: group.name, value:
group.id }))}
data-test="groups-select"
/>
</FormItem>
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ef9ec2ee-0372-42cd-88f2-4f34ac390c91/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ef9ec2ee-0372-42cd-88f2-4f34ac390c91?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ef9ec2ee-0372-42cd-88f2-4f34ac390c91?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ef9ec2ee-0372-42cd-88f2-4f34ac390c91?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ef9ec2ee-0372-42cd-88f2-4f34ac390c91)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:70ee9275-0aa0-4e48-9734-423e3c32c9d4 -->
[](70ee9275-0aa0-4e48-9734-423e3c32c9d4)
##########
superset-frontend/src/features/roles/RoleFormItems.tsx:
##########
@@ -21,14 +21,22 @@
import { Input } from 'src/components/Input';
import { t } from '@superset-ui/core';
import { FC } from 'react';
-import { FormattedPermission, UserObject } from './types';
+import { GroupObject } from 'src/pages/GroupsList';
+import AsyncSelect from 'src/components/Select/AsyncSelect';
+import { FormattedPermission } from './types';
+import { fetchUserOptions } from '../groups/utils';
interface PermissionsFieldProps {
permissions: FormattedPermission[];
}
+interface GroupsFieldProps {
+ groups: GroupObject[];
+}
+
interface UsersFieldProps {
- users: UserObject[];
+ addDangerToast: (msg: string) => void;
+ loading: boolean;
}
Review Comment:
### Missing prop documentation in UsersFieldProps interface <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The purpose and usage context of 'addDangerToast' and 'loading' props are
not documented in the interface.
###### Why this matters
Without clear documentation, other developers may incorrectly use these
props or misunderstand their impact on the component's behavior.
###### Suggested change ∙ *Feature Preview*
```typescript
interface UsersFieldProps {
/** Callback to display error messages during user fetch operations */
addDangerToast: (msg: string) => void;
/** Indicates whether a user fetch operation is in progress */
loading: boolean;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0156c461-d7b6-49b4-9fc2-701292dbbcd4/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0156c461-d7b6-49b4-9fc2-701292dbbcd4?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0156c461-d7b6-49b4-9fc2-701292dbbcd4?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0156c461-d7b6-49b4-9fc2-701292dbbcd4?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0156c461-d7b6-49b4-9fc2-701292dbbcd4)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:5b0ac344-5356-4133-80cf-3b4889476f39 -->
[](5b0ac344-5356-4133-80cf-3b4889476f39)
##########
superset-frontend/src/constants.ts:
##########
@@ -204,3 +204,8 @@ export enum FilterPlugins {
TimeColumn = 'filter_timecolumn',
TimeGrain = 'filter_timegrain',
}
+
+export enum Actions {
+ CREATE = 'create',
+ UPDATE = 'update',
+}
Review Comment:
### Overly Generic Enum Name <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The 'Actions' enum name is too generic and doesn't provide context about its
specific use case in the constants file.
###### Why this matters
Generic names can lead to naming conflicts and make code harder to
understand as the codebase grows. It's unclear what these actions are for,
making the code less maintainable.
###### Suggested change ∙ *Feature Preview*
Rename the enum to be more specific to its use case, for example:
```typescript
export enum ResourceActions {
CREATE = 'create',
UPDATE = 'update',
}
```
or
```typescript
export enum CrudActions {
CREATE = 'create',
UPDATE = 'update',
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1292d29d-88b4-4f65-b71d-6bd978aa9891/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1292d29d-88b4-4f65-b71d-6bd978aa9891?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1292d29d-88b4-4f65-b71d-6bd978aa9891?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1292d29d-88b4-4f65-b71d-6bd978aa9891?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1292d29d-88b4-4f65-b71d-6bd978aa9891)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:26e9ebf7-733e-4e4b-9719-c26d425b01a8 -->
[](26e9ebf7-733e-4e4b-9719-c26d425b01a8)
##########
superset-frontend/src/features/roles/RoleFormItems.tsx:
##########
@@ -58,13 +66,28 @@
</FormItem>
);
-export const UsersField: FC<UsersFieldProps> = ({ users }) => (
+export const UsersField = ({ addDangerToast, loading }: UsersFieldProps) => (
<FormItem name="roleUsers" label={t('Users')}>
+ <AsyncSelect
+ name="roleUsers"
+ mode="multiple"
+ placeholder={t('Select users')}
+ options={(filterValue, page, pageSize) =>
+ fetchUserOptions(filterValue, page, pageSize, addDangerToast)
+ }
+ loading={loading}
+ data-test="roles-select"
+ />
Review Comment:
### Incorrect test selector for UsersField component <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The data-test attribute for the UsersField component is incorrectly set to
'roles-select' instead of 'users-select', which is inconsistent with the
component's purpose.
###### Why this matters
This inconsistency could cause automated tests to fail or become unreliable,
as test selectors would be looking for the wrong attribute value.
###### Suggested change ∙ *Feature Preview*
Change the data-test attribute to match the component's purpose:
```typescript
export const UsersField = ({ addDangerToast, loading }: UsersFieldProps) => (
<FormItem name="roleUsers" label={t('Users')}>
<AsyncSelect
name="roleUsers"
mode="multiple"
placeholder={t('Select users')}
options={(filterValue, page, pageSize) =>
fetchUserOptions(filterValue, page, pageSize, addDangerToast)
}
loading={loading}
data-test="users-select"
/>
</FormItem>
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/201940e4-a4f4-4f63-9588-5d565b358c22/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/201940e4-a4f4-4f63-9588-5d565b358c22?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/201940e4-a4f4-4f63-9588-5d565b358c22?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/201940e4-a4f4-4f63-9588-5d565b358c22?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/201940e4-a4f4-4f63-9588-5d565b358c22)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:29955aa3-5f2d-4b40-9531-614fe9d05068 -->
[](29955aa3-5f2d-4b40-9531-614fe9d05068)
##########
superset/views/groups.py:
##########
@@ -0,0 +1,34 @@
+# 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 flask_appbuilder import permission_name
+from flask_appbuilder.api import expose
+from flask_appbuilder.security.decorators import has_access
+
+from superset.superset_typing import FlaskResponse
+
+from .base import BaseSupersetView
+
+
+class GroupsListView(BaseSupersetView):
+ route_base = "/"
+ class_permission_name = "security"
+
+ @expose("/list_groups/")
+ @has_access
+ @permission_name("read")
+ def list(self) -> FlaskResponse:
+ return super().render_app_template()
Review Comment:
### Empty view implementation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The list method lacks any groups-specific logic and just returns a generic
template render without passing any context or group data.
###### Why this matters
This implementation doesn't follow the Interface Segregation Principle as it
provides no specific functionality for group listing. The view becomes a
pass-through with no real purpose.
###### Suggested change ∙ *Feature Preview*
Implement proper groups listing logic:
```python
def list(self) -> FlaskResponse:
groups = self.get_groups() # Add method to fetch groups
return super().render_app_template(groups=groups)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/503de822-1944-4575-9e87-16bcbd40b9de/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/503de822-1944-4575-9e87-16bcbd40b9de?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/503de822-1944-4575-9e87-16bcbd40b9de?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/503de822-1944-4575-9e87-16bcbd40b9de?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/503de822-1944-4575-9e87-16bcbd40b9de)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:588fe9d4-35cc-4f39-8372-459814077c78 -->
[](588fe9d4-35cc-4f39-8372-459814077c78)
##########
superset/views/groups.py:
##########
@@ -0,0 +1,34 @@
+# 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 flask_appbuilder import permission_name
+from flask_appbuilder.api import expose
+from flask_appbuilder.security.decorators import has_access
+
+from superset.superset_typing import FlaskResponse
+
+from .base import BaseSupersetView
+
+
+class GroupsListView(BaseSupersetView):
+ route_base = "/"
+ class_permission_name = "security"
+
+ @expose("/list_groups/")
+ @has_access
+ @permission_name("read")
Review Comment:
### Ambiguous permission name <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The permission name 'read' is too generic and doesn't clearly indicate what
resource is being read.
###### Why this matters
Generic permission names make it harder to understand and audit access
control requirements.
###### Suggested change ∙ *Feature Preview*
```python
@permission_name("read_groups")
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa578fc7-101d-407c-9f94-596d4ac1782b/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa578fc7-101d-407c-9f94-596d4ac1782b?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa578fc7-101d-407c-9f94-596d4ac1782b?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa578fc7-101d-407c-9f94-596d4ac1782b?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa578fc7-101d-407c-9f94-596d4ac1782b)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:e83b0744-887e-42c9-a8e1-85843978c7a0 -->
[](e83b0744-887e-42c9-a8e1-85843978c7a0)
##########
superset-frontend/src/features/roles/RoleFormItems.tsx:
##########
@@ -58,13 +66,28 @@
</FormItem>
);
-export const UsersField: FC<UsersFieldProps> = ({ users }) => (
+export const UsersField = ({ addDangerToast, loading }: UsersFieldProps) => (
<FormItem name="roleUsers" label={t('Users')}>
+ <AsyncSelect
+ name="roleUsers"
+ mode="multiple"
+ placeholder={t('Select users')}
+ options={(filterValue, page, pageSize) =>
+ fetchUserOptions(filterValue, page, pageSize, addDangerToast)
+ }
+ loading={loading}
+ data-test="roles-select"
+ />
+ </FormItem>
+);
+
+export const GroupsField: FC<GroupsFieldProps> = ({ groups }) => (
+ <FormItem name="roleGroups" label={t('Groups')}>
<Select
mode="multiple"
- name="roleUsers"
- options={users.map(user => ({ label: user.username, value: user.id }))}
- data-test="users-select"
+ name="roleGroups"
+ options={groups.map(group => ({ label: group.name, value: group.id }))}
Review Comment:
### Memoize Select Options <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The groups array is mapped on every render to create options for the Select
component, creating new objects each time.
###### Why this matters
Unnecessarily recreating option objects on each render increases memory
allocation and can impact performance, especially with large groups arrays.
###### Suggested change ∙ *Feature Preview*
Use useMemo to memoize the options array to prevent unnecessary recreations:
```typescript
const groupOptions = useMemo(
() => groups.map(group => ({ label: group.name, value: group.id })),
[groups]
);
// Then use in Select
options={groupOptions}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7115cb8b-a015-43b8-971d-d1b921580e7f/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7115cb8b-a015-43b8-971d-d1b921580e7f?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7115cb8b-a015-43b8-971d-d1b921580e7f?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7115cb8b-a015-43b8-971d-d1b921580e7f?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7115cb8b-a015-43b8-971d-d1b921580e7f)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:8ee228eb-89aa-469c-871d-283dd8424c7e -->
[](8ee228eb-89aa-469c-871d-283dd8424c7e)
--
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]
