bito-code-review[bot] commented on code in PR #44359:
URL: https://github.com/apache/superset/pull/44359#discussion_r4030593871
##########
superset-frontend/src/dashboard/components/gridComponents/Row/Row.test.tsx:
##########
@@ -269,6 +278,57 @@ test('should increment the depth of its children', () => {
);
});
+test('row droptarget height should shrink after the tallest chart in the row
is resized smaller (regression for #37644)', () => {
+ // Emulates a flex row sized to fit-content: its clientHeight is at least
+ // as tall as its tallest child, including a droptarget sibling whose
+ // height was explicitly set to a pixel value by a prior render. This
+ // mirrors how a real browser lays out GridRow and its droptargets.
+ const clientHeightDescriptor = Object.getOwnPropertyDescriptor(
+ Element.prototype,
+ 'clientHeight',
+ );
+ let trueContentHeight = 300;
+
+ Object.defineProperty(Element.prototype, 'clientHeight', {
+ configurable: true,
+ get(this: Element) {
+ if (!this.classList.contains('grid-row')) return 0;
+ const appliedHeights = Array.from(
+ this.querySelectorAll<HTMLElement>('.empty-droptarget--vertical'),
+ ).map(el => parseFloat(el.style.height) || 0);
+ return Math.max(trueContentHeight, 0, ...appliedHeights);
+ },
+ });
+
+ try {
+ const { container, rerender } = setup({ editMode: true });
+ const getDroptargetHeight = () =>
+
container.querySelector<HTMLElement>('.empty-droptarget--vertical')?.style
+ .height;
+
+ // Sanity: the droptarget renders a height while the row's tallest
+ // chart is large.
+ expect(getDroptargetHeight()).toBeTruthy();
+
+ // The tallest chart in the row shrinks, then something (e.g. hovering
+ // the row's own HoverMenu while resizing) causes Row to re-render.
+ trueContentHeight = 100;
+ rerender(<Row {...props} editMode component={{ ...props.component }} />);
+
+ // The droptarget must track the shrunk content instead of staying
+ // pinned to the chart's prior (larger) measured height.
+ expect(getDroptargetHeight()).not.toBe('300px');
+ } finally {
+ if (clientHeightDescriptor) {
+ Object.defineProperty(
+ Element.prototype,
+ 'clientHeight',
+ clientHeightDescriptor,
+ );
+ }
+ }
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inert test mock</b></div>
<div id="fix">
This test mocks `Element.prototype.clientHeight`, but this PR removed the
`containerHeight` state and `useLayoutEffect` that read `clientHeight` from
`Row.tsx`; the droptarget height is now hardcoded to `'100%'`. The mock is
inert, so `toBeTruthy()` and `not.toBe('300px')` pass trivially regardless of
the regression fix, giving false confidence. Assert the real behavior (height
stays `'100%'`) or remove the mock.
</div>
</div>
<small><i>Code Review Run #ac02b6</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
