kgabryje commented on PR #41350:
URL: https://github.com/apache/superset/pull/41350#issuecomment-4955953118
Reviewed this (including independent verification of the core claims against
the `superset-ui-core`/dashboard formData-assembly source, not just the PR
description). The `formData.extraFormData?.time_grain_sqla ?? timeGrainSqla`
precedence is correct: `ChartProps` only shallow-camelCases `formData`, so
`extra_form_data` becomes `extraFormData` while the nested `time_grain_sqla`
stays snake_case, and tracing
`getFormDataWithExtraFilters.ts`/`getFormDataWithDashboardContext.ts` confirms
the override genuinely needs the fallback on both the dashboard and Explore
paths. No security/perf concerns, and the `minInterval`/`maxInterval` fallback
asymmetry is intentional (their "no constraint" defaults differ), not a bug.
Two things worth addressing before merge:
1. **Test coverage gap on the actual wiring.** The new tests in
`formatters.test.ts` only exercise `getTooltipTimeFormatter` directly with
manually-passed args. Nothing exercises `resolvedTimeGrain`/the
`extra_form_data` override through the full `transformProps` pipeline for
either `Timeseries` or `MixedTimeseries` — and `MixedTimeseries` has no
tooltip/grain coverage at all despite duplicating the same logic. This is the
exact seam most likely to regress silently (a future change to
`convertKeysToCamelCase`'s depth, or a field-name typo, would still pass every
current test). Would be good to add one pipeline-level test per plugin
asserting the override reaches the rendered tooltip.
2. **`UPDATING.md` wording is slightly overstated.**
- "matching the behavior already applied to the x-axis" — the tooltip
(`getTimeFormatter(undefined, grain)`) and the x-axis (`getSmartDateFormatter`)
both become grain-aware, but don't render identical labels/ranges (e.g. tooltip
shows `2021 Q1` / weekly ranges, x-axis shows more compact adaptive labels).
Might be worth softening to "grain-aware" rather than "matching."
- "Charts that pin an explicit tooltip time format are unaffected" —
`SMART_DATE_ID` ("Adaptive formatting") is both the control's default *and* an
explicitly selectable option (`D3_TIME_FORMAT_OPTIONS[0]`), and it's routed
through the new grain-aware branch either way. So a chart where a user
explicitly picked "Adaptive formatting" is also affected — only a genuinely
custom d3 format string is guaranteed unaffected.
Logic itself looks solid; these are both fairly quick fixes.
--
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]
