EnxDev commented on PR #41390: URL: https://github.com/apache/superset/pull/41390#issuecomment-4834297025
## EnxDev's Review Agent โ apache/superset#41390 ยท HEAD 348d924 **request changes** โ the fix approach is sound, but it breaks the TypeScript build (CI red on `lint-frontend` + `validate-frontend`). The diagnosis and design are good: attaching the formatter to the row-data object (non-enumerable, resolved from `node.data`) is the right way to make it survive client-side sorting, and the index alignment matches what the old positional lookup already assumed at transform time. One blocking type error stops it from compiling. ### ๐ด Functional - **`src/renderers/NumericCellRenderer.tsx:172-173`** ยท _High_ โ `getRowBasicColorFormatter()` returns `RowFormatters | undefined`, so `rowFormatter?.mainArrow` / `rowFormatter?.arrowColor?.toLowerCase()` are `string | undefined`, assigned to `arrow`/`arrowColor` which are typed `string` (`let arrow = ''`). `tsc --build` fails with TS2322 (confirmed: both `lint-frontend` and `validate-frontend` are red). The old expression resolved to `string` because every link in the chain was non-nullable, so the new helper's `| undefined` is what broke it. Fix: `arrow = rowFormatter?.mainArrow ?? '';` and `arrowColor = rowFormatter?.arrowColor?.toLowerCase() ?? '';`. **regression test:** the build itself โ CI must go green; the existing jest suite already passed, so the only gap is type-checking. ### ๐ก Should-fix - **`test/utils/getRowBasicColorFormatter.test.ts:26`** โ `const positional = [green, red] as any;` uses `any`, which the project bans. Type it as `RowFormatters[]` (or the helper's param type) so the test also guards the signature. ### ๐ Praise - `src/utils/getRowBasicColorFormatter.ts` / `transformProps.ts:704` โ non-enumerable attach is a clean choice: keeps the formatter travelling with the row through sort while staying out of CSV exports, cross-filters, and spreads, and the positional fallback is a sensible safety net. Test for non-enumerability is a nice touch. <!-- enxdev-review-agent:348d924 --> _Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 348d924._ -- 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]
