EnxDev commented on PR #37482:
URL: https://github.com/apache/superset/pull/37482#issuecomment-4809633106
## EnxDev's Review Agent โ apache/superset#37482 ยท HEAD 8e606db
**comment** โ Core fix is correct for the server-paginated (2-query) case it
targets; one latent numbering bug remains for 3+ queries. Already approved by
@rusackas; CI mid-run.
The real fix is `cachedWhen[index]` in `getCachedTitle` โ previously
`t('Cached %s', cachedWhen)` interpolated the whole array (comma-joined times),
which is exactly the "Cached a month ago" mislabel on Query 2. Good catch, and
the `|| ''` dead branch from the earlier commit was correctly dropped.
### ๐ก Should-fix
- **`SliceHeaderControls/index.tsx:391`** โ `new Set(...)` dedupes titles by
value, but the later `t('Query %s: %s', index + 1, item)` numbers off the
*post-dedup* index. For 3+ queries with a partial duplicate (e.g. `["Cached
X","Cached X","Cached Y"]` โ `["Cached X","Cached Y"]`), query 3's time renders
as "Query 2", mislabeling which query was cached. The 2-query target case is
unaffected (distinct โ kept in order; identical โ collapses to one, as
intended), so this is latent โ but it's the unresolved concern raised inline
and the diff doesn't address it. Only collapse when *all* titles are identical;
otherwise keep the full per-query list so `index + 1` stays aligned. (test: 3
queries, `isCached=[true,true,true]`, two sharing a cache time โ assert "Query
3:" shows query 3's value.)
### ๐ต Nits
- `SliceHeaderControls.test.tsx:716` ("Should deduplicate identical cache
times") โ only asserts `/Cached/` is present; it won't catch a regression where
dedup stops collapsing. Add `expect(screen.queryByText(/Query
1:/)).not.toBeInTheDocument()` to actually guard the collapse.
- `SliceHeaderControls.test.tsx:752` ("three or more queries") โ
`cachedDttm3 = ''` with `isCached[2] = true` yields `dayjs.utc('').fromNow()` โ
"Cached Invalid Date". The assertion passes, but the fixture is an impossible
state; use a real timestamp so the test reflects reality.
### ๐ Praise
- `SliceHeaderControls.test.tsx` โ solid per-query coverage (single/multi,
cached/fetched, dedup, 3+ mixed), and async `hover` + `findByText` addresses
the earlier flaky-test flags.
<!-- enxdev-review-agent:8e606db -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 8e606db._
--
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]
