EnxDev commented on PR #38657: URL: https://github.com/apache/superset/pull/38657#issuecomment-4809575888
## EnxDev's Review Agent โ apache/superset#38657 ยท HEAD ad515f1 **Request changes** โ the formula is a no-op for the short panels it's meant to fix, so #37286 isn't addressed, and the new test hides that. ### ๐ด Functional - **`src/Timeseries/transformProps.ts:1001`** โ `Math.max(TIMESERIES_CONSTANTS.zoomBottom, Math.floor(height * 0.08))` with `zoomBottom = 30` (`constants.ts:41`) crosses over at `height = 375`. For **every height โค 375px** it resolves to exactly `30` โ identical to the old fixed offset. Issue #37286 is a *short panel* clipping bug, i.e. precisely the range where this changes nothing. The only heights it affects are tall charts (>375px), which weren't the problem and lose a little plot area as the slider shifts up. So the fix doesn't fix the linked issue. What's the intended direction here โ should the offset *shrink* (or the grid's `gridOffsetBottomZoomable` adapt) on short charts so the slider actually fits? - **`test/Timeseries/transformProps.test.ts:1635`** โ the test passes only because it compares 300px (`bottom=30`) to 600px (`bottom=48`); the inequality is driven entirely by the tall chart. It asserts `smallBottom !== largeBottom` but never checks the short-panel value, so it would still pass with the no-op-on-short-charts behavior above. It gives false confidence that the bug is fixed. (test: assert the actual short-panel `bottom` and that it differs from the pre-PR `30`.) ### ๐ก Should-fix - **`test/Timeseries/transformProps.test.ts:1655`** โ assert the exact computed values (e.g. `30` and `48`) rather than just inequality, so the formula is actually pinned by the test. ### ๐ต Nits - `src/Timeseries/transformProps.ts:1001` โ Gantt/MixedTimeseries still use the fixed `zoomBottom`; if the responsive offset is kept, apply it consistently (already raised by a bot). - `test/Timeseries/transformProps.test.ts:1647` โ `as any[]` casts; prefer typing against the dataZoom option shape. ### ๐ Praise - Added a focused `transformProps` test with `zoomable: true` in response to reviewer feedback โ right instinct, just needs sharper assertions. <!-- enxdev-review-agent:ad515f1 --> _Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD ad515f1._ -- 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]
