korbit-ai[bot] commented on code in PR #34233:
URL: https://github.com/apache/superset/pull/34233#discussion_r2217265301
##########
superset-frontend/src/utils/downloadAsImage.ts:
##########
@@ -70,9 +70,41 @@ export default function downloadAsImage(
return true;
};
+ // Check if element is scrollable
+ const isScrollable = hasScrollableDescendant(elementToPrint as
HTMLElement);
Review Comment:
### Missing Debug Logs for Export Logic Path <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Missing debug logging for scrollable element detection, which is a key
decision point in the export logic.
###### Why this matters
When troubleshooting export issues, it would be helpful to know whether the
scrollable content path was taken and what dimensions were detected.
###### Suggested change ∙ *Feature Preview*
```typescript
const isScrollable = hasScrollableDescendant(elementToPrint as HTMLElement);
console.debug(`Element ${selector} scrollable status: ${isScrollable},
dimensions: ${elementToPrint.scrollWidth}x${elementToPrint.scrollHeight}`);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5f2953da-5540-4c17-b82c-88311ee2e05e/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5f2953da-5540-4c17-b82c-88311ee2e05e?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5f2953da-5540-4c17-b82c-88311ee2e05e?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5f2953da-5540-4c17-b82c-88311ee2e05e?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5f2953da-5540-4c17-b82c-88311ee2e05e)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ae9964a3-5462-4a6f-9ff8-4da4d4fac175 -->
[](ae9964a3-5462-4a6f-9ff8-4da4d4fac175)
##########
superset-frontend/src/utils/downloadAsImage.ts:
##########
@@ -70,9 +70,41 @@
return true;
};
+ // Check if element is scrollable
+ const isScrollable = hasScrollableDescendant(elementToPrint as
HTMLElement);
+ let targetElement = elementToPrint as HTMLElement;
+ let cloned: HTMLElement | null = null;
+ const extraPadding = theme?.sizeUnit ? theme.sizeUnit * 2 : 16;
Review Comment:
### Unexplained Magic Number <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Magic number 16 is used without explanation of its significance.
###### Why this matters
Future developers won't understand why 16 was chosen as the default padding
value, making maintenance risky.
###### Suggested change ∙ *Feature Preview*
```typescript
const DEFAULT_PADDING = 16; // Default padding in pixels when theme is not
available
const extraPadding = theme?.sizeUnit ? theme.sizeUnit * 2 : DEFAULT_PADDING;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b27d8d76-218d-466b-9f16-74c4c3e81782/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b27d8d76-218d-466b-9f16-74c4c3e81782?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b27d8d76-218d-466b-9f16-74c4c3e81782?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b27d8d76-218d-466b-9f16-74c4c3e81782?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b27d8d76-218d-466b-9f16-74c4c3e81782)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:b9c62fe0-677f-4490-8110-268a8f5be129 -->
[](b9c62fe0-677f-4490-8110-268a8f5be129)
##########
superset-frontend/src/utils/downloadAsImage.ts:
##########
@@ -83,6 +115,38 @@
})
.catch((e: Error) => {
console.error('Creating image failed', e);
+ })
+ .finally(() => {
+ if (cloned) {
+ document.body.removeChild(cloned);
+ }
});
};
}
+
+/**
+ * Check if an element or any of its child elements is scrollable
+ *
+ * @param el - The HTMLElement to check for scrollable descendants.
+ * @returns `true` if any descendant has scrollable overflow; otherwise
`false`.
+ */
+function hasScrollableDescendant(el: HTMLElement): boolean {
+ const treeWalker = document.createTreeWalker(el, NodeFilter.SHOW_ELEMENT);
Review Comment:
### Multiple forced reflows <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
TreeWalker is being used to check every element's scroll dimensions, which
requires accessing layout properties that force reflow on each iteration.
###### Why this matters
Each scrollHeight and clientHeight access forces the browser to recalculate
layout, potentially causing significant performance overhead when checking many
elements.
###### Suggested change ∙ *Feature Preview*
Cache scroll-related values and batch DOM reads to minimize reflows.
Consider using IntersectionObserver or a more efficient way to detect
scrollable content.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/25f24d65-ca5b-4155-843a-f7d50dcf5719/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/25f24d65-ca5b-4155-843a-f7d50dcf5719?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/25f24d65-ca5b-4155-843a-f7d50dcf5719?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/25f24d65-ca5b-4155-843a-f7d50dcf5719?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/25f24d65-ca5b-4155-843a-f7d50dcf5719)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:8b54c2cb-a50e-4e32-9d09-307643f448a9 -->
[](8b54c2cb-a50e-4e32-9d09-307643f448a9)
##########
superset-frontend/src/utils/downloadAsImage.ts:
##########
@@ -70,9 +70,41 @@
return true;
};
+ // Check if element is scrollable
+ const isScrollable = hasScrollableDescendant(elementToPrint as
HTMLElement);
+ let targetElement = elementToPrint as HTMLElement;
+ let cloned: HTMLElement | null = null;
+ const extraPadding = theme?.sizeUnit ? theme.sizeUnit * 2 : 16;
+ if (isScrollable) {
+ cloned = elementToPrint.cloneNode(true) as HTMLElement;
+ cloned.style.overflow = 'visible';
+ cloned.style.maxHeight = 'none';
+ cloned.style.height = 'auto';
+ cloned.style.width = `${elementToPrint.scrollWidth}px`;
+
+ cloned.querySelectorAll<HTMLElement>('*').forEach(el => {
+ // eslint-disable-next-line no-param-reassign
+ el.style.overflow = 'visible';
+ // eslint-disable-next-line no-param-reassign
+ el.style.maxHeight = 'none';
+ // eslint-disable-next-line no-param-reassign
+ el.style.height = 'auto';
+ });
Review Comment:
### Extract scrollable element styling logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The styling logic for scrollable elements is mixed within the main download
function and contains repetitive style assignments.
###### Why this matters
Violates the Single Responsibility Principle and DRY principle, making the
code harder to maintain and modify.
###### Suggested change ∙ *Feature Preview*
Extract the styling logic into a separate function:
```typescript
function prepareScrollableElement(element: HTMLElement): HTMLElement {
const cloned = element.cloneNode(true) as HTMLElement;
const styleProps = { overflow: 'visible', maxHeight: 'none', height:
'auto' };
Object.assign(cloned.style, styleProps);
cloned.style.width = `${element.scrollWidth}px`;
cloned.querySelectorAll<HTMLElement>('*')
.forEach(el => Object.assign(el.style, styleProps));
return cloned;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fcc09318-7a7e-44ae-b41a-7951852bc5e9/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fcc09318-7a7e-44ae-b41a-7951852bc5e9?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fcc09318-7a7e-44ae-b41a-7951852bc5e9?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fcc09318-7a7e-44ae-b41a-7951852bc5e9?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fcc09318-7a7e-44ae-b41a-7951852bc5e9)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f84faf23-deeb-4ba5-a9b6-be3c7f7d0fb1 -->
[](f84faf23-deeb-4ba5-a9b6-be3c7f7d0fb1)
##########
superset-frontend/src/utils/downloadAsImage.ts:
##########
@@ -70,9 +70,41 @@
return true;
};
+ // Check if element is scrollable
+ const isScrollable = hasScrollableDescendant(elementToPrint as
HTMLElement);
+ let targetElement = elementToPrint as HTMLElement;
+ let cloned: HTMLElement | null = null;
+ const extraPadding = theme?.sizeUnit ? theme.sizeUnit * 2 : 16;
+ if (isScrollable) {
+ cloned = elementToPrint.cloneNode(true) as HTMLElement;
+ cloned.style.overflow = 'visible';
+ cloned.style.maxHeight = 'none';
+ cloned.style.height = 'auto';
+ cloned.style.width = `${elementToPrint.scrollWidth}px`;
+
+ cloned.querySelectorAll<HTMLElement>('*').forEach(el => {
Review Comment:
### Inefficient DOM traversal <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using querySelectorAll('*') followed by forEach creates unnecessary
iteration over every DOM element, including those that don't need style
modifications.
###### Why this matters
This operation has O(n) complexity where n is the total number of DOM
elements, potentially causing performance issues with large DOM trees. Many
elements processed may not even have overflow or height/maxHeight styles.
###### Suggested change ∙ *Feature Preview*
Target only elements that could have scroll-related styles using a more
specific selector like '[style*="overflow"], [style*="height"]' or create a
list of specific element types/classes that need modification.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b90533e-1c77-4654-b4f1-9c4ed9dd3c88/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b90533e-1c77-4654-b4f1-9c4ed9dd3c88?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b90533e-1c77-4654-b4f1-9c4ed9dd3c88?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b90533e-1c77-4654-b4f1-9c4ed9dd3c88?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b90533e-1c77-4654-b4f1-9c4ed9dd3c88)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:88fc5161-9d6d-43cd-9d66-d5bbcb38269a -->
[](88fc5161-9d6d-43cd-9d66-d5bbcb38269a)
--
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]
