EnxDev commented on PR #43938:
URL: https://github.com/apache/superset/pull/43938#issuecomment-5784865979
Read through this one properly — it's a lot of surface area, but the core of
it holds up. The locale problem that dominated the earlier rounds does look
genuinely resolved rather than papered over: stored group keys are the literal
`Main`, the chart keys are `t('Main')`, and `parseTimeComparisonSlot` /
`resolveTimeComparisonSlotKeys` bridge the two in both directions.
`isHeaderGroupsTimeComparisonEnabled` now matches the plugins'
`isUsingTimeComparison` exactly (`time_compare` + aggregate + `Values`),
`canSaveHeaderGroup` recurses into children, and `expandGroupColumnKey`
short-circuits on an exact match before trying the `%` fallback, so the three
things Copilot raised there are actually fixed in the diff and not just
claimed. Nice touch stashing `allow_rearrange_columns` via the `visibility`
gate instead of leaving it live to fight the group ordering.
Six notes inline. The two I'd want addressed before merge are the prune
effect in `HeaderGroupsControl` — it dispatches a non-`programmatic` `onChange`
from mount, so opening a saved chart can flag unsaved changes the user never
made, which is exactly what the `{ programmatic: true }` on your
`ControlPanelsContainer` effect avoids — and `getClaimedColumnKeys` claiming
`%${identifier}`, which locks a percent metric out of every other group as soon
as its base metric is grouped. The rest are smaller: the auto-group label going
stale when a metric is renamed, edit mode silently reverting an invalid edit
with no feedback, a hardcoded `3px` radius, and a design question about
table-specific logic landing in `ControlPanelsContainer`.
Two things outside the diff worth a thought. This is a user-facing feature
with no `docs/` update and no feature flag — is that deliberate? And the
Testing Instructions section of the description is empty, which makes it harder
for anyone to reproduce the time-comparison interaction by hand. Test coverage
itself is solid; the header-group utils in particular are exercised well.
<!-- enxdev-review-human:ba72da1 -->
--
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]
