codeant-ai-for-open-source[bot] commented on code in PR #42097:
URL: https://github.com/apache/superset/pull/42097#discussion_r3648771822
##########
superset-frontend/src/visualizations/TimeTable/TimeTable.tsx:
##########
@@ -43,6 +46,30 @@ const TimeTable = ({
rows,
url = '',
}: TimeTableProps) => {
+ // Hide TableView as soon as the window starts resizing, and only show it
+ // again once resizing has stopped for RESIZE_DEBOUNCE_MS.
+ const resizeTimerRef = useRef<ReturnType<typeof setTimeout> | undefined>(
+ undefined,
+ );
+ const [isSizeStable, setIsSizeStable] = useState(true);
+
+ useEffect(() => {
+ const handleResize = () => {
+ setIsSizeStable(false);
+ clearTimeout(resizeTimerRef.current);
+ resizeTimerRef.current = setTimeout(() => {
+ setIsSizeStable(true);
+ }, RESIZE_DEBOUNCE_MS);
+ };
+
+ window.addEventListener('resize', handleResize);
Review Comment:
**Suggestion:** The listener observes only browser-level `window` resize
events, so resizing a dashboard tile or other containing element without
changing the window dimensions will not hide and debounce the table. Use an
appropriate container resize observation mechanism if tile resizing is part of
the supported behavior. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dashboard tile resizing can still trigger table layout work.
- ⚠️ Container-resized tables may retain visible jank.
- ⚠️ The optimization covers viewport resizing, not all supported resize
paths.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render `TimeTable` through its component entry point at
`superset-frontend/src/visualizations/TimeTable/TimeTable.tsx:46-48` inside
a dashboard
tile.
2. Resize the dashboard tile using the tile’s container/grid resize
interaction rather
than changing the browser viewport.
3. The effect at `TimeTable.tsx:56-71` has registered only
`window.addEventListener('resize', handleResize)` at line 65 and does not
observe the tile
element with `ResizeObserver`.
4. Because no window resize event is necessarily generated by a
container-only resize,
`handleResize` is not called, `isSizeStable` remains true at line 54, and
the `TableView`
at lines 165-166 remains visible and active throughout the tile resize.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6c80f95343b841f5b0c1058f3ffd1bd4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6c80f95343b841f5b0c1058f3ffd1bd4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/visualizations/TimeTable/TimeTable.tsx
**Line:** 65:65
**Comment:**
*Possible Bug: The listener observes only browser-level `window` resize
events, so resizing a dashboard tile or other containing element without
changing the window dimensions will not hide and debounce the table. Use an
appropriate container resize observation mechanism if tile resizing is part of
the supported behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42097&comment_hash=de643c1080c23163eddb7d2fb130c50f44cfbb4455114e094fc2f264cf7fde96&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42097&comment_hash=de643c1080c23163eddb7d2fb130c50f44cfbb4455114e094fc2f264cf7fde96&reaction=dislike'>👎</a>
##########
superset-frontend/src/visualizations/TimeTable/TimeTable.tsx:
##########
@@ -115,21 +142,25 @@ const TimeTable = ({
});
}, [columnConfigs, data, rowType, rows, url]);
- const defaultSort =
- rowType === 'column' && columnConfigs.length
- ? [
- {
- id: columnConfigs[0].key,
- desc: true,
- },
- ]
- : [];
+ const defaultSort = useMemo(
+ () =>
+ rowType === 'column' && columnConfigs.length
+ ? [
+ {
+ id: columnConfigs[0].key,
+ desc: true,
+ },
+ ]
+ : [],
+ [rowType, columnConfigs],
+ );
return (
<TimeTableStyles
data-test="time-table"
className={className}
height={height}
+ hideTable={!isSizeStable}
Review Comment:
**Suggestion:** The resize state only applies `display: none` to the
wrapper; `TableView` remains mounted and is still part of the React tree, so
its expensive render and internal table work are not actually skipped during
resizing. Conditionally render `TableView` only when `isSizeStable` is true if
the goal is to avoid those operations. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Window resizing still processes mounted `TableView` renders.
- ⚠️ Time-series tables with many columns may retain resize jank.
- ⚠️ The PR’s intended table-unmount optimization is not achieved.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render the `TimeTable` component defined in
`superset-frontend/src/visualizations/TimeTable/TimeTable.tsx:46-48` with
normal table
data and columns.
2. Drag the browser window edge so `window` emits a resize event; the
handler at
`TimeTable.tsx:56-63` calls `setIsSizeStable(false)` on every event.
3. React re-renders the component, but `TimeTable.tsx:165-166` still renders
`<TableView>`; only the wrapper receives `display: none` through `hideTable`
at line 163.
4. Observe that `TableView` remains mounted and can still execute its
render/update
lifecycle during resize, so the expensive table work described in the PR is
not reliably
skipped; conditionally rendering `TableView` only when `isSizeStable` is
true would
achieve the stated unmounting behavior.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1c0ba53565254bc5958c5d5d95bdb643&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1c0ba53565254bc5958c5d5d95bdb643&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/visualizations/TimeTable/TimeTable.tsx
**Line:** 163:163
**Comment:**
*Possible Bug: The resize state only applies `display: none` to the
wrapper; `TableView` remains mounted and is still part of the React tree, so
its expensive render and internal table work are not actually skipped during
resizing. Conditionally render `TableView` only when `isSizeStable` is true if
the goal is to avoid those operations.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42097&comment_hash=bceabc55bc5db8cb4918d25cb6098be37bb445624106312e4e96a58e43b72c8a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42097&comment_hash=bceabc55bc5db8cb4918d25cb6098be37bb445624106312e4e96a58e43b72c8a&reaction=dislike'>👎</a>
--
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]
