Copilot commented on code in PR #37482:
URL: https://github.com/apache/superset/pull/37482#discussion_r2733356657
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -325,16 +325,18 @@ const SliceHeaderControls = (
const updatedWhen = updatedDttm
? (extendedDayjs.utc(updatedDttm) as any).fromNow()
: '';
- const getCachedTitle = (itemCached: boolean) => {
+ const getCachedTitle = (itemCached: boolean, index: number) => {
if (itemCached) {
- return t('Cached %s', cachedWhen);
+ return t('Cached %s', cachedWhen[index]);
}
if (updatedWhen) {
return t('Fetched %s', updatedWhen);
}
return '';
};
- const refreshTooltipData = [...new Set(isCached.map(getCachedTitle) || '')];
+ const refreshTooltipData = [
+ ...new Set(isCached.map((itemCached, index) => getCachedTitle(itemCached,
index)) || ''),
Review Comment:
The `|| ''` fallback inside `new Set(isCached.map((itemCached, index) =>
getCachedTitle(itemCached, index)) || '')` is unreachable, because
`Array.prototype.map` always returns an array and arrays are always truthy in
JavaScript/TypeScript. To simplify and avoid confusion, you can drop the `||
''` and rely directly on the mapped array.
```suggestion
...new Set(isCached.map((itemCached, index) =>
getCachedTitle(itemCached, index))),
```
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -325,16 +325,18 @@ const SliceHeaderControls = (
const updatedWhen = updatedDttm
? (extendedDayjs.utc(updatedDttm) as any).fromNow()
: '';
- const getCachedTitle = (itemCached: boolean) => {
+ const getCachedTitle = (itemCached: boolean, index: number) => {
if (itemCached) {
- return t('Cached %s', cachedWhen);
+ return t('Cached %s', cachedWhen[index]);
}
Review Comment:
This change introduces new behavior where each cached query row uses its own
`cachedDttm` entry via `cachedWhen[index]`, but there is no unit test in
`SliceHeaderControls.test.tsx` verifying the per-query cache/fetch tooltip text
(including server-pagination scenarios with multiple queries). Given that this
component already has Jest tests, consider adding a test that renders a table
viz with multiple `isCached`/`cachedDttm` entries and asserts the correct
per-query tooltip strings so regressions in this logic are caught automatically.
--
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]
