ayush-sharaf commented on code in PR #42563:
URL: https://github.com/apache/superset/pull/42563#discussion_r3673751713
##########
superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx:
##########
@@ -108,6 +111,19 @@ export default function DrillDetailModal({
findPermission('can_explore', 'Superset', state.user?.roles),
);
+ // Unlike DrillByModal, this component stays mounted for the lifetime of the
+ // chart and is toggled via `showModal`, so the event is gated on the modal
+ // actually being opened rather than firing on mount.
+ useEffect(() => {
+ if (showModal) {
+ dispatch(
+ logEvent(LOG_ACTIONS_DRILL_TO_DETAIL_MODAL_OPENED, {
+ slice_id: chartId,
+ }),
+ );
+ }
+ }, [dispatch, showModal, chartId]);
Review Comment:
Thanks for the review. I looked into this and I don't believe it's reachable
with the current usage, so I'd rather not add the extra state.
`DrillDetailModal` is rendered from two places, and in both the `chartId` is
fixed for the lifetime of the mounted instance:
- `SliceHeaderControls` renders it with `chartId={slice.slice_id}`, and that
component is rendered per slice.
- `ChartContextMenu` renders it with `chartId={id}`, and that component is
rendered per chart inside `ChartRenderer`.
So there is no path where a mounted instance's `chartId` changes while
`showModal` stays `true`. On top of that the modal is modal — it blocks
interaction with the dashboard underneath while open — so a chart swap under an
open modal isn't reachable either.
Given that, a `useRef` transition guard would be extra state to keep in sync
with no behavioural difference today. I'd prefer to keep the effect as it is
and revisit if a future caller ever shares one modal instance across charts.
Worth noting the suggested snippet also initialises the ref with
`useRef(showModal)`, which would swallow the event for a component that mounts
with the modal already open. If we do add a guard later, it should start from
`false`.
The existing tests cover the behaviour: one asserts the event fires with the
correct `slice_id` when the modal is opened, and another asserts nothing is
logged while it is closed.
--
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]
