fix(explore): keep the annotation layer modal usable in a small viewport - #42614
Conversation
Two defects in the "Add annotation layer" modal, both surfacing when a chart
is picked as the annotation source.
Annotation Slice Configuration fields never rendered
----------------------------------------------------
`fetchSliceData` and `fetchAppliedChart` read the source chart's form data as
`JSON.parse(result.query_context).form_data`. `query_context` is a derived
cache, backfilled the first time a chart is opened in Explore, so a chart that
has never been opened there has `query_context: null`. `JSON.parse(null)`
returns `null`, reading `.form_data` off it throws, the promise rejects
unhandled and `slice` is never set: the fields stay absent and Apply/OK stay
disabled. Nearly every chart is affected, since the column is only ever
populated as a side effect of visiting Explore.
The form data now resolves through a helper that prefers
`query_context.form_data` and falls back to the chart's saved `params`, which
hold the same form data and are always present. Parsing is guarded and both
requests handle rejection instead of failing silently.
Modal overflowed the viewport, hiding Display configuration and Apply/OK
----------------------------------------------------------------------
The sections were laid out in an unbounded, non-wrapping flex row, so the third
one pushed the popover past a narrow viewport - an embedding iframe, typically.
Nothing could recover from it: antd only configures `shiftX`/`shiftY` for the
four base placements, and ControlPopover computes corner placements, which get
`{adjustX, adjustY}` alone. Those can flip a popup across its trigger but never
nudge it back into view, so when neither side fitted the popover simply stayed
off screen with its footer unreachable.
ControlPopover now opts into shifting, so a popup that does not fit is pulled
back rather than left hanging. That alone still leaves `popupOffsetX` behind,
because the shift discounts it, so the section row is additionally capped to the
room left beside the control panel and wraps within it. The panel's edge is the
anchor rather than the popover's own, which trails its content width by several
frames and would feed each cap into the next measurement.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dosubot
Bot
added
change:frontend
Code Review Agent Run #742a90Actionable Suggestions - 0Additional Suggestions - 2
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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42614 +/- ##
==========================================
+ Coverage 65.40% 65.41% +0.01%
==========================================
Files 2810 2810
Lines 159102 159163 +61
Branches 36273 36287 +14
==========================================
+ Hits 104068 104124 +56
- Misses 52993 52998 +5
Partials 2041 2041
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
amaannawab923
left a comment
amaannawab923
left a comment
There was a problem hiding this comment.
nice one, this is a solid fix. the params fallback for null query_context makes sense and i like that you added the catch so it stops failing silently. the width stuff is fiddly but reads well and good on you for actually testing it with a mocked ResizeObserver. few thoughts below, none of them blocking.
…-and-slice-config
Code Review Agent Run #196b06Actionable Suggestions - 0Additional Suggestions - 1
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
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 |
|
Bito Automatic Review Skipped – PR Already Merged |
2 participants
SUMMARY
Fixes two issues in the Add annotation layer modal that occur after selecting a chart as the annotation source.
Annotation Slice Configuration fields not rendering
fetchSliceDataandfetchAppliedChartread the source chart's form data from:JSON.parse(result.query_context).form_dataHowever,
query_contextis a derived cache that is only populated after a chart has been opened in Explore. For charts that have never been opened there,query_contextcan therefore benull.Although
JSON.parse(null)returnsnull, accessing.form_datathen throws. The promise rejection was not handled,slicewas never set, and the Annotation Slice Configuration fields remained hidden, leaving Apply/OK disabled.Form data is now resolved through a helper that:
query_context.form_datawhen available;params, which contain the persisted form data;Modal overflowing the viewport
The modal could overflow narrow viewports, particularly when Superset is embedded in an iframe, causing the Display configuration section and Apply/OK actions to become unreachable.
The sections were rendered in an unbounded, non-wrapping flex row. In addition, Ant Design only enables
shiftX/shiftYfor the four base placements, whileControlPopoveruses corner placements that only received{ adjustX, adjustY }. These options can flip the popup relative to its trigger, but cannot shift it back inside the viewport when neither side has enough space.ControlPopovernow explicitly enables shifting.Because rc-trigger's shift calculation discounts
popupOffsetX, shifting alone is not sufficient. The section row is therefore also capped to the available space beside the control panel and allowed to wrap when necessary.The control panel edge is used as the sizing anchor rather than the popover itself, avoiding unstable measurements while the popover width is still responding to its content.
The width constraint only takes effect when space is limited. At normal desktop widths, the natural width remains unchanged and the existing three-column layout is preserved.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Measured in a 1160×859 iframe:
before-missing-fields.mp4
after-missing-fields.mp4
before-overflow.mp4
after-overflow.mp4
TESTING INSTRUCTIONS
Missing slice configuration fields:
query_contextis stillNULL(select id from slices where query_context is null). It must be a viz type that can be an annotation source Table or Table V2 and be owned, created or favourited by you, since the Chart dropdown filters onchart_owned_created_favored_by_me.params, and Apply/OK enable once a time column is set. On master the fields never appear and Apply/OK stay disabled.Viewport overflow:
query_context, so the third section appears.ADDITIONAL INFORMATION