korbit-ai[bot] commented on code in PR #32996:
URL: https://github.com/apache/superset/pull/32996#discussion_r2026832522
##########
superset-frontend/src/features/rls/RowLevelSecurityModal.tsx:
##########
@@ -321,6 +343,55 @@
[],
);
+ const createDropdownRender = useCallback(
+ (entityType: 'tables' | 'roles') => (menu: React.ReactNode) => (
+ <>
+ <StyledDropdownContainer>
+ <StyledDropdownButton
+ type="button"
+ onClick={() => {
+ const query = rison.encode({
+ page: 0,
+ page_size: -1,
+ });
+ SupersetClient.get({
+ endpoint:
`/api/v1/rowlevelsecurity/related/${entityType}?q=${query}`,
+ }).then(response => {
+ const allEntities = response.json.result.map(
+ (item: { value: number; text: string }) => ({
+ key: item.value,
+ label: item.text,
+ value: item.value,
+ }),
+ );
+ updateRuleState(entityType, allEntities || []);
+ });
+ }}
+ >
Review Comment:
### Complex Inline Event Handler <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The onClick handler in createDropdownRender contains complex nested logic
including API calls and data transformation, making it difficult to understand
the button's behavior at a glance.
###### Why this matters
Complex inline handlers reduce code scanability and make it harder to
maintain or debug the component's behavior.
###### Suggested change ∙ *Feature Preview*
Extract the handler into a separate named function:
```typescript
const handleSelectAll = useCallback(async (entityType: 'tables' | 'roles')
=> {
const query = rison.encode({ page: 0, page_size: -1 });
const response = await SupersetClient.get({
endpoint: `/api/v1/rowlevelsecurity/related/${entityType}?q=${query}`,
});
const allEntities = response.json.result.map(
(item: { value: number; text: string }) => ({
key: item.value,
label: item.text,
value: item.value,
}),
);
updateRuleState(entityType, allEntities || []);
}, [updateRuleState]);
// In render:
<StyledDropdownButton
type="button"
onClick={() => handleSelectAll(entityType)}
>
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/aec2f385-9097-459b-92e1-732ae93a6927/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/aec2f385-9097-459b-92e1-732ae93a6927?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/aec2f385-9097-459b-92e1-732ae93a6927?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/aec2f385-9097-459b-92e1-732ae93a6927?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/aec2f385-9097-459b-92e1-732ae93a6927)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:e71ff292-24e6-47e0-8df4-996c26b1f87f -->
[](e71ff292-24e6-47e0-8df4-996c26b1f87f)
##########
superset-frontend/src/features/rls/RowLevelSecurityModal.tsx:
##########
@@ -321,6 +343,55 @@ function RowLevelSecurityModal(props:
RowLevelSecurityModalProps) {
[],
);
+ const createDropdownRender = useCallback(
+ (entityType: 'tables' | 'roles') => (menu: React.ReactNode) => (
+ <>
+ <StyledDropdownContainer>
+ <StyledDropdownButton
+ type="button"
+ onClick={() => {
+ const query = rison.encode({
+ page: 0,
+ page_size: -1,
+ });
+ SupersetClient.get({
+ endpoint:
`/api/v1/rowlevelsecurity/related/${entityType}?q=${query}`,
+ }).then(response => {
+ const allEntities = response.json.result.map(
+ (item: { value: number; text: string }) => ({
+ key: item.value,
+ label: item.text,
+ value: item.value,
+ }),
+ );
+ updateRuleState(entityType, allEntities || []);
+ });
Review Comment:
### Redundant API Call for Select All <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The 'Select all' functionality makes a new API call to fetch all entities,
even though we're already loading entities via AsyncSelect's options loader.
###### Why this matters
This creates unnecessary network overhead and server load. The data could be
reused from the existing AsyncSelect data loading mechanism.
###### Suggested change ∙ *Feature Preview*
Modify the code to use the existing loadTableOptions/loadRoleOptions
functions with a large page size to fetch all entities. Cache the results in a
ref or state to avoid repeated fetches:
```typescript
const fetchAllEntities = useCallback(async (entityType: 'tables' | 'roles')
=> {
const loader = entityType === 'tables' ? loadTableOptions :
loadRoleOptions;
const { data } = await loader('', 0, 1000);
updateRuleState(entityType, data || []);
}, [loadTableOptions, loadRoleOptions]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b5cb1462-ca76-4d2c-ad5c-f7277458c3c3/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b5cb1462-ca76-4d2c-ad5c-f7277458c3c3?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b5cb1462-ca76-4d2c-ad5c-f7277458c3c3?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b5cb1462-ca76-4d2c-ad5c-f7277458c3c3?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b5cb1462-ca76-4d2c-ad5c-f7277458c3c3)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:01d865b0-64c4-4084-b074-bd7794e52f36 -->
[](01d865b0-64c4-4084-b074-bd7794e52f36)
##########
superset-frontend/src/features/rls/RowLevelSecurityModal.tsx:
##########
@@ -321,6 +343,55 @@
[],
);
+ const createDropdownRender = useCallback(
+ (entityType: 'tables' | 'roles') => (menu: React.ReactNode) => (
+ <>
+ <StyledDropdownContainer>
+ <StyledDropdownButton
+ type="button"
+ onClick={() => {
+ const query = rison.encode({
+ page: 0,
+ page_size: -1,
+ });
+ SupersetClient.get({
+ endpoint:
`/api/v1/rowlevelsecurity/related/${entityType}?q=${query}`,
+ }).then(response => {
Review Comment:
### Missing error handling in API request <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The API request lacks error handling, which could lead to unhandled promise
rejections if the request fails.
###### Why this matters
Users won't receive feedback if the select all operation fails, and the
application might be left in an inconsistent state.
###### Suggested change ∙ *Feature Preview*
Add proper error handling to the API request:
```typescript
SupersetClient.get({
endpoint: `/api/v1/rowlevelsecurity/related/${entityType}?q=${query}`,
})
.then(response => {
const allEntities = response.json.result.map(
(item: { value: number; text: string }) => ({
key: item.value,
label: item.text,
value: item.value,
}),
);
updateRuleState(entityType, allEntities || []);
})
.catch(error => {
addDangerToast(t('Failed to fetch all entities'));
console.error('Failed to fetch all entities:', error);
});
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a9455657-0271-42ad-b579-944ee781ca3d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a9455657-0271-42ad-b579-944ee781ca3d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a9455657-0271-42ad-b579-944ee781ca3d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a9455657-0271-42ad-b579-944ee781ca3d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a9455657-0271-42ad-b579-944ee781ca3d)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:837a40f3-acee-4027-9003-43d9a5add010 -->
[](837a40f3-acee-4027-9003-43d9a5add010)
--
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]
