EnxDev commented on PR #37229:
URL: https://github.com/apache/superset/pull/37229#issuecomment-4809677232
## EnxDev's Review Agent โ apache/superset#37229 ยท HEAD e6e5600
**comment** โ Clean, well-tested fix for compound-key tooltip labels. One
latent crash on missing `label_map` and a multi-word-label gap before LGTM.
### ๐ด Functional
- **`utils/forecast.ts:91` + `MixedTimeseries/transformProps.ts:150-159`** โ
`addLabelMapToVerboseMap`'s first param has no default, and
`labelMap`/`labelMapB` are destructured without defaults
(`transformProps.ts:150,152`) and passed straight in.
`Object.entries(label_map)` (`forecast.ts:132`) throws `Cannot convert
undefined or null to object` if a query omits `label_map`. The Timeseries path
guards this (`label_map = {}`, `Timeseries/transformProps.ts:211`) and existing
Mixed code already optional-chains `labelMap?.[seriesName]`
(`transformProps.ts:461`) โ so the asymmetry is real. Default the helper's
first param to `{}`? (test: `transformProps`/`addLabelMapToVerboseMap` with
`label_map` undefined)
### ๐ก Should-fix
- **`utils/forecast.ts:144`** โ `key.split(' ')` tokenizes on spaces, so any
`verboseMap` key containing a space (quoted identifier, custom metric label
like `Order Total`) never matches and the compound series keeps its raw label.
Single-token keys work; multi-word labels silently don't. Run the replacement
over the full `key` (or tokenize on `, `) instead of single spaces.
- **`utils/forecast.test.ts`** โ no case covers the missing/empty
`label_map` path โ the exact crash in the Functional item. Add one; it's red
before the fix above, green after.
### ๐ต Nits
- `utils/forecast.ts:41,60` โ `replaceLabelIfExists` JSDoc claims it
requires "at least one alphabetic character" and ignores numeric-only matches,
but the body (`forecast.ts:78-80`) has no such check; the `"12"` vs `"123"`
example only holds via the word boundaries. Align the doc with the code.
- `spec/fixtures/mockTimeSeries.ts:162,269` โ blanket `as any` violates the
no-`any` rule. Type the fixtures against
`EchartsTimeseriesChartProps`/`EchartsMixedTimeseriesProps` (or use narrow
casts on the few loose fields) rather than casting the whole object.
### ๐ Praise
- `utils/forecast.test.ts` โ the `(?<!\w)โฆ(?!\w)` boundary matching cleanly
avoids substring collisions (`account` not matching inside `testing_account`,
`count` not inside `account`), and the test cases prove it. Nice.
<!-- enxdev-review-agent:e6e5600 -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD e6e5600._
--
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]
