EnxDev commented on PR #41031: URL: https://github.com/apache/superset/pull/41031#issuecomment-4901380546
## EnxDev's Review Agent — apache/superset#41031 · HEAD 16bc695 lgtm — re-review after the branch was refreshed. The delta since my last pass is just master merged in (i18n backfills, dep bumps, unrelated fixes); the PR's own logic is unchanged and nothing regressed on the rebase. One stale title clause still outstanding. _Supersedes my [prior review](https://github.com/apache/superset/pull/41031#issuecomment-4901054300) (HEAD 8b41124)._ **Re-verified at this HEAD.** The tokens are still live, not dead: every key the builder emits — `headerFontSize`, `headerFontWeight`, `rowHeight`/`headerHeight`, `borderRadius`/`wrapperBorderRadius`, `oddRowBackgroundColor` — is a real ag-grid v34 theme parameter and is spread into `themeQuartz.withParams()` in `ThemedAgGridReact`. `SUPERSET_CUSTOM_TOKENS` registration keeps them out of antd's token algorithm, and `SupersetTheme = SharedAntdTokens & SupersetSpecificTokens` so the util typechecks. No token set → builder returns `undefined` → `withParams` is skipped → defaults are provably unchanged. ### 🟡 Should-fix (carried over — still open) - **PR title** — still reads *"…and fix modal border-radius clipping"*, but that modal change was (correctly) dropped and no modal file is touched. The squashed `feat:` changelog line will advertise a fix that isn't in the PR. Drop the clause; if the modal clipping is still worth fixing, do it in its own PR without the global `overflow: hidden`. - **Docs** — the 5 deployer-facing `resultsGrid*` tokens still aren't documented. Add them next to the neighboring theme tokens in the token docs / `types.ts`. ### 🔵 Nits - **`TabbedSqlEditors/index.tsx:47-83`** — confirmed the `!important`→`&&` swap is safe: Superset sets no `hashPriority`, so antd v5 wraps its Tabs rules in `:where()` (near-zero specificity) and the doubled-class `&&` (0,2,0) out-specifies them cleanly. Screenshot backs up "no visual change." Worth one glance in dark mode, but no action needed. - **`GridTable/index.tsx:143-158`** — with a row-height token set, `rowHeight`/`headerHeight` land via both `gridOptions` and the forwarded `themeOverrides` → `withParams`. Harmless (same value; the grid option is authoritative), just redundant. ### 🙌 Praise - A pure, typed util with 9 focused regression tests, and the buggy global-`overflow: hidden` modal change dropped rather than patched over. Clean. <!-- enxdev-review-agent:16bc695 --> _Reviewed by EnxDev's Review Agent — @EnxDev · HEAD 16bc695._ -- 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]
