You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Vitor-Avila (via GitHub)" <gi...@apache.org> on 2025/07/06 01:23:44 UTC
[PR] chore: Improve performance to load the chart properties modal [superset]
Vitor-Avila opened a new pull request, #34079:
URL: https://github.com/apache/superset/pull/34079
### SUMMARY
**_Note: This is currently on draft pending https://github.com/dpgaspar/Flask-AppBuilder/pull/2372 to be included in a new FAB release._**
Currently, the chart properties modal fires two requests to `/api/v1/chart/${ID}`: one to load tags and another to load owners. Additionally, the request returns all model columns, including the `json_metadata` value for all dashboards that contain this chart, which can considerably affect performance.
This PR improves the performance by reducing to one API call + selecting only the columns neeed by the modal.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
**Before**
<img width="1790" alt="image" src="https://github.com/user-attachments/assets/7eecba07-d17c-4049-ab20-2d350812f394" />
**After**
<img width="1792" alt="image" src="https://github.com/user-attachments/assets/51aa5749-1c66-422e-b5b2-5399e1a1a1ae" />
### TESTING INSTRUCTIONS
1. Access the Charts list.
2. Open the browser's Developer Tools and access the Network tab.
3. Hover over any chart and click on the pencil icon to open the properties modal.
4. Confirm that a single API call is fired.
5. Confirm the API call includes the `select_columns` argument.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: Improve performance to load the chart properties modal [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #34079:
URL: https://github.com/apache/superset/pull/34079#discussion_r2195229907
##########
superset-frontend/src/explore/components/PropertiesModal/PropertiesModal.test.tsx:
##########
@@ -422,3 +419,28 @@ test('"Certification details" should not be empty when saved', async () => {
);
});
});
+
+test('Should display only custom tags when tagging system is enabled', async () => {
+ const mockIsFeatureEnabled = isFeatureEnabled as jest.MockedFunction<
+ typeof isFeatureEnabled
+ >;
+ mockIsFeatureEnabled.mockImplementation(
+ flag => flag === FeatureFlag.TaggingSystem,
+ );
+
+ const props = createProps();
+ renderModal(props);
+
+ await waitFor(() => {
Review Comment:
forgot to say that this is done now
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: Improve performance to load the chart properties modal [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #34079:
URL: https://github.com/apache/superset/pull/34079#discussion_r2190525375
##########
superset-frontend/src/explore/components/PropertiesModal/PropertiesModal.test.tsx:
##########
@@ -422,3 +419,28 @@ test('"Certification details" should not be empty when saved', async () => {
);
});
});
+
+test('Should display only custom tags when tagging system is enabled', async () => {
+ const mockIsFeatureEnabled = isFeatureEnabled as jest.MockedFunction<
+ typeof isFeatureEnabled
+ >;
+ mockIsFeatureEnabled.mockImplementation(
+ flag => flag === FeatureFlag.TaggingSystem,
+ );
+
+ const props = createProps();
+ renderModal(props);
+
+ await waitFor(() => {
Review Comment:
Sure, I'll take a look
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: Improve performance to load the chart properties modal [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #34079:
URL: https://github.com/apache/superset/pull/34079#discussion_r2190485456
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -114,6 +123,12 @@ function PropertiesModal({
label: `${owner.first_name} ${owner.last_name}`,
})),
);
+ if (isFeatureEnabled(FeatureFlag.TaggingSystem)) {
+ const customTags = chart.tags?.filter(
+ (tag: TagType) => tag.type === 1,
Review Comment:
Makes me wonder what type `1` is? Is this documented in the code somewhere?
##########
superset-frontend/src/explore/components/PropertiesModal/PropertiesModal.test.tsx:
##########
@@ -422,3 +419,28 @@ test('"Certification details" should not be empty when saved', async () => {
);
});
});
+
+test('Should display only custom tags when tagging system is enabled', async () => {
+ const mockIsFeatureEnabled = isFeatureEnabled as jest.MockedFunction<
+ typeof isFeatureEnabled
+ >;
+ mockIsFeatureEnabled.mockImplementation(
+ flag => flag === FeatureFlag.TaggingSystem,
+ );
+
+ const props = createProps();
+ renderModal(props);
+
+ await waitFor(() => {
Review Comment:
Nit. Can we prefer `findBy` for the following ones?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: Improve performance to load the chart properties modal [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila merged PR #34079:
URL: https://github.com/apache/superset/pull/34079
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: Improve performance to load the chart properties modal [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #34079:
URL: https://github.com/apache/superset/pull/34079#discussion_r2190538737
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -114,6 +123,12 @@ function PropertiesModal({
label: `${owner.first_name} ${owner.last_name}`,
})),
);
+ if (isFeatureEnabled(FeatureFlag.TaggingSystem)) {
+ const customTags = chart.tags?.filter(
+ (tag: TagType) => tag.type === 1,
Review Comment:
Yup. We have different type of tags -- the type is stored as an enum implemented here: https://github.com/apache/superset/blob/master/superset/tags/models.py#L61-L77
This filter is needed so that the UI only shows custom tags (user-created tags) and not the system tags.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: Improve performance to load the chart properties modal [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #34079:
URL: https://github.com/apache/superset/pull/34079#discussion_r2195186389
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -101,11 +100,21 @@ function PropertiesModal({
});
}
- const fetchChartOwners = useCallback(
- async function fetchChartOwners() {
+ const fetchChartProperties = useCallback(
+ async function fetchChartProperties() {
+ const queryParams = rison.encode({
+ select_columns: [
+ 'owners.id',
+ 'owners.first_name',
+ 'owners.last_name',
+ 'tags.id',
+ 'tags.name',
+ 'tags.type',
+ ],
+ });
try {
const response = await SupersetClient.get({
- endpoint: `/api/v1/chart/${slice.slice_id}`,
+ endpoint: `/api/v1/chart/${slice.slice_id}?q=${queryParams}`,
});
Review Comment:
### Unmemoized Dependency in useCallback <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The fetchChartProperties callback is recreated on every render because slice.slice_id is in the dependency array but isn't memoized.
###### Why this matters
This can lead to unnecessary re-renders and API calls when the component updates for unrelated reasons.
###### Suggested change ∙ *Feature Preview*
Memoize the slice.slice_id value using useMemo or move it to component state if it's expected to change:
```typescript
const sliceId = useMemo(() => slice.slice_id, [slice.slice_id]);
const fetchChartProperties = useCallback(
async function fetchChartProperties() {
// ... existing code
},
[sliceId]
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/679c71fb-c76c-4155-b545-aa1c47a9dd48/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/679c71fb-c76c-4155-b545-aa1c47a9dd48?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/679c71fb-c76c-4155-b545-aa1c47a9dd48?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/679c71fb-c76c-4155-b545-aa1c47a9dd48?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/679c71fb-c76c-4155-b545-aa1c47a9dd48)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:7c5d223c-1d4b-4255-b8d8-ebc47cef8ba9 -->
[](7c5d223c-1d4b-4255-b8d8-ebc47cef8ba9)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: Improve performance to load the chart properties modal [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #34079:
URL: https://github.com/apache/superset/pull/34079#discussion_r2187880243
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -114,6 +124,12 @@
label: `${owner.first_name} ${owner.last_name}`,
})),
);
+ if (isFeatureEnabled(FeatureFlag.TaggingSystem)) {
+ const customTags = chart.tags?.filter(
+ (tag: TagType) => tag.type === 1,
+ );
Review Comment:
### Replace magic number with named constant <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The magic number '1' is used to filter custom tags without explanation of its significance.
###### Why this matters
Magic numbers make code harder to understand and maintain as their meaning is not immediately clear to other developers.
###### Suggested change ∙ *Feature Preview*
```typescript
const CUSTOM_TAG_TYPE = 1;
// Add comment explaining what CUSTOM_TAG_TYPE represents
const customTags = chart.tags?.filter(
(tag: TagType) => tag.type === CUSTOM_TAG_TYPE,
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e1442f87-3ef9-452a-86bb-c0accf49d781/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e1442f87-3ef9-452a-86bb-c0accf49d781?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e1442f87-3ef9-452a-86bb-c0accf49d781?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e1442f87-3ef9-452a-86bb-c0accf49d781?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e1442f87-3ef9-452a-86bb-c0accf49d781)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d74bfa63-aad7-4b20-8a5c-207b1f5a3f8c -->
[](d74bfa63-aad7-4b20-8a5c-207b1f5a3f8c)
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -101,11 +101,21 @@ function PropertiesModal({
});
}
- const fetchChartOwners = useCallback(
- async function fetchChartOwners() {
+ const fetchChartProperties = useCallback(
+ async function fetchChartProperties() {
+ const queryParams = rison.encode({
+ select_columns: [
+ 'owners.id',
+ 'owners.first_name',
+ 'owners.last_name',
+ 'tags.id',
+ 'tags.name',
+ 'tags.type',
+ ],
+ });
try {
const response = await SupersetClient.get({
- endpoint: `/api/v1/chart/${slice.slice_id}`,
+ endpoint: `/api/v1/chart/${slice.slice_id}?q=${queryParams}`,
});
Review Comment:
### Unvalidated Path Parameter in API Request <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The slice_id parameter is being directly interpolated into the URL string without validation or sanitization.
###### Why this matters
If slice_id is manipulated, it could potentially lead to path traversal attacks or allow unauthorized access to other chart data if authorization is not properly enforced on the server side.
###### Suggested change ∙ *Feature Preview*
Validate slice_id is a positive integer before using it in the API request:
```typescript
if (!Number.isInteger(slice.slice_id) || slice.slice_id <= 0) {
throw new Error('Invalid chart ID');
}
const endpoint = `/api/v1/chart/${slice.slice_id}?q=${queryParams}`;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31c2bb5a-1579-4679-ae93-735be6cfd96f/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31c2bb5a-1579-4679-ae93-735be6cfd96f?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31c2bb5a-1579-4679-ae93-735be6cfd96f?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31c2bb5a-1579-4679-ae93-735be6cfd96f?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31c2bb5a-1579-4679-ae93-735be6cfd96f)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:7ff5f656-4578-4372-b910-de9090bea364 -->
[](7ff5f656-4578-4372-b910-de9090bea364)
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -114,6 +124,12 @@
label: `${owner.first_name} ${owner.last_name}`,
})),
);
+ if (isFeatureEnabled(FeatureFlag.TaggingSystem)) {
+ const customTags = chart.tags?.filter(
+ (tag: TagType) => tag.type === 1,
+ );
+ setTags(customTags);
+ }
Review Comment:
### Unsafe array operation on potentially undefined value <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code assumes chart.tags will always be an array when filtering, but it might be undefined since it's accessed with optional chaining.
###### Why this matters
If chart.tags is undefined, calling filter on undefined will cause a runtime error and crash the application.
###### Suggested change ∙ *Feature Preview*
Add a fallback empty array to ensure filter operation is safe:
```typescript
if (isFeatureEnabled(FeatureFlag.TaggingSystem)) {
const customTags = (chart.tags ?? []).filter(
(tag: TagType) => tag.type === 1,
);
setTags(customTags);
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/72e4841d-fe6c-4584-9242-114e2549e015/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/72e4841d-fe6c-4584-9242-114e2549e015?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/72e4841d-fe6c-4584-9242-114e2549e015?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/72e4841d-fe6c-4584-9242-114e2549e015?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/72e4841d-fe6c-4584-9242-114e2549e015)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ff89e346-da01-4eb5-87a7-290aa85a6c06 -->
[](ff89e346-da01-4eb5-87a7-290aa85a6c06)
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -101,11 +101,21 @@
});
}
- const fetchChartOwners = useCallback(
- async function fetchChartOwners() {
+ const fetchChartProperties = useCallback(
+ async function fetchChartProperties() {
+ const queryParams = rison.encode({
+ select_columns: [
+ 'owners.id',
+ 'owners.first_name',
+ 'owners.last_name',
+ 'tags.id',
+ 'tags.name',
+ 'tags.type',
+ ],
+ });
Review Comment:
### Use typed constants for API column names <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The column names for the API query are hardcoded as an array of strings without type safety or constants.
###### Why this matters
Without type safety or constants, developers may make typos when referencing these columns, leading to runtime errors that could have been caught during development.
###### Suggested change ∙ *Feature Preview*
```typescript
const CHART_PROPERTY_COLUMNS = {
OWNER_ID: 'owners.id',
OWNER_FIRST_NAME: 'owners.first_name',
OWNER_LAST_NAME: 'owners.last_name',
TAG_ID: 'tags.id',
TAG_NAME: 'tags.name',
TAG_TYPE: 'tags.type',
} as const;
const queryParams = rison.encode({
select_columns: Object.values(CHART_PROPERTY_COLUMNS),
});
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/44822f1e-008e-4977-8056-f480073d3095/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/44822f1e-008e-4977-8056-f480073d3095?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/44822f1e-008e-4977-8056-f480073d3095?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/44822f1e-008e-4977-8056-f480073d3095?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/44822f1e-008e-4977-8056-f480073d3095)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:218f261a-f44a-4989-b6e6-7955717f76e7 -->
[](218f261a-f44a-4989-b6e6-7955717f76e7)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org