Repository navigation
fix(explore-chart-panel): last queried at overlap - #37501
ramiroaquinoromero wants to merge 5 commits into
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
ramiroaquinoromero
changed the title
dosubot
Bot
added
change:frontend
f238dd5 to
ed7f103
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Code Review Agent Run #76fbf3Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Also can you add some testing steps and highlight the bug on the summary. I attached the issue onto the pr but it would be easy to miss what's wrong from the videos |
reynoldmorel
left a comment
reynoldmorel
left a comment
There was a problem hiding this comment.
I think this solution adds a small flickering according to your video recordings. I know this will sound very rookie on my end, but did you try adding minHeight and minWidth to the parent container? Using resize observer consumes a lot of resources in the client side, so probably avoiding as much as possible would be good.
Not sure about what you tried already, but just asking to make sure we tried everything we knew before going this way
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #988724
Actionable Suggestions - 1
-
superset-frontend/src/explore/components/ExploreChartPanel/useResizeDetectorByObserver.ts - 1
- Broken initial chart render · Line 28-33
Review Details
-
Files reviewed - 3 · Commit Range:
ed7f103..791dd36- superset-frontend/src/explore/components/ExploreChartPanel/ExploreChartPanel.test.jsx
- superset-frontend/src/explore/components/ExploreChartPanel/index.tsx
- superset-frontend/src/explore/components/ExploreChartPanel/useResizeDetectorByObserver.ts
-
Files skipped - 0
-
Tools
- Eslint (Linter) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
791dd36 to
4a36e69
Compare
|
@msyavuz Please review it. |
Code Review Agent Run #7d6ddcActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Superset uses Git pre-commit hooks courtesy of pre-commit. To install run the following: A series of checks will now run when you make a git commit. Alternatively it is possible to run pre-commit by running pre-commit manually: |
|
Can we also update the description to get more of context on what the issue was, what was the root cause and what the fix is? |
…ast-queried-at-overlap
SUMMARY
Open up an existing chart from the charts list (note that it's easier to see this bug on a bar chart, area chart, line chart etc than it is on something like a Big Number)
PROBLEM
The chart container didn’t have a minimum size, so in a flex layout it could shrink too much and the chart ended up drawing in a tiny or unstable area. On top of that, the resize logic ran as soon as the component mounted and kept firing every 100 ms, so we were constantly measuring and re-rendering. That made the chart area jump around and caused the flicker. We were also using && to only show the chart when we had width and height, so when one of them was still 0 we could briefly render a “0” on screen and add to the layout mess.
The “Last queried at” label didn’t have its own reserved row it was just another flex item so when the panel was small, that row could get squeezed or end up on top of the chart instead of below it.
So in practice: no minimum size for the chart area, too much measuring right from the start, and no fixed space for the label. That’s what led to the clipping, flicker, and overlap.
###SOLUTION
We set minimum dimensions for the container. We turned off resize on mount and slowed updates to 250 ms, and we use a ternary so we only render the chart when we have real dimensions. We gave "Last queried at" a fixed row so it doesn’t overlap the chart. That fixes the clipping, overlap, and flicker.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
before.mp4
After
after.mp4
After fix
after.mp4
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION