fitzee commented on code in PR #43838:
URL: https://github.com/apache/superset/pull/43838#discussion_r4001585941
##########
superset-frontend/src/features/versionHistory/RelatedUpdateRow.tsx:
##########
@@ -144,6 +152,13 @@ export default function RelatedUpdateRow({
const headline = relatedHeadline(record);
const entityName = entityDisplayName(record);
+ // The "Dataset used by N charts updated" phrasing summarizes siblings the
+ // row cannot name inline; the impact payload carries them for the hover
+ // detail (sc-119775), mirroring the rolled-up-names tooltip above.
+ const impactCharts = record.impact?.affected_charts ?? [];
Review Comment:
**Cross-version inconsistency: headline can advertise charts the tooltip
never reveals.** `impactCharts` reads `affected_charts` (typed optional "for
older backends that predate the field"), while the headline `relatedHeadline`
keys off `impact.charts` (display.ts:381). Against a backend that emits
`impact.charts` but not `affected_charts`, a dataset record with `charts = 4`
renders "Dataset used by 4 charts updated: X" while `impactCharts = []` makes
the tooltip `null` — the row promises detail that hovering never shows. The
type doc already anticipates the older-backend case, so it's worth closing the
loop here: e.g. fall back to a count-only tooltip (or suppress the impact-aware
headline phrasing) when `affected_charts` is absent but `charts > 0`.
##########
superset-frontend/src/features/versionHistory/RelatedUpdateRow.tsx:
##########
@@ -121,10 +127,12 @@ export default function RelatedUpdateRow({
</IconWrapper>
<Content>
<Tooltip
- title={rollupEntityNames.map((name, index) => (
- // eslint-disable-next-line react/no-array-index-key
- <div key={index}>{name || t('Untitled')}</div>
- ))}
+ title={tooltipNameList(
Review Comment:
**Coverage gap: rolled-up rows never surface the new affected-charts
tooltip.** This rollup branch (`if (rollupEntityNames.length > 1)` at line 121)
returns early, before `impactCharts` is even computed (line 158). So when
several same-kind dataset records roll into one row, the tooltip shows only the
rolled-up entity names — any `record.impact.affected_charts` on those records
is dropped, exactly on the busiest rows the feature is most useful for. If
exposing affected charts on rolled-up impacts is out of scope, fine — but it's
currently undocumented, so a reader can't tell whether it's intentional or an
oversight. Worth a one-line comment either way (and, if desired, merging the
affected-chart names into the rollup tooltip).
##########
superset-frontend/src/features/versionHistory/RelatedUpdateRow.tsx:
##########
@@ -159,20 +174,45 @@ export default function RelatedUpdateRow({
<Icon iconSize="l" />
</IconWrapper>
<Content>
- <Headline>
- {nameIndex >= 0 ? (
- <>
- {headline.slice(0, nameIndex)}
- <NameLink type="button" onClick={() => onOpen?.(record)}>
- {entityName}
- </NameLink>
- {headline.slice(nameIndex + record.entity_name.length)}
- </>
- ) : (
- headline
- )}
- {record.entity_deleted && ` (${t('deleted')})`}
- </Headline>
+ <Tooltip
Review Comment:
**Nit (cleanliness): the `Headline` is now always wrapped in a `Tooltip`,
even for the common `impact === null` row where `title` is `null`.** For
chart-related records (no impact), `impactCharts` is empty and the title is
`null`, yet every such row still mounts a `Tooltip` wrapper. Antd
short-circuits the popup on an empty title so there's no functional bug, but on
long activity lists it's needless per-row wrapping. Rendering the `Tooltip`
only when `impactCharts.length > 0` (and the bare `Headline` otherwise) keeps
the non-impact path as light as before.
--
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]
