anamitraadhikari commented on PR #41031: URL: https://github.com/apache/superset/pull/41031#issuecomment-4900844144
> ## EnxDev's Review Agent — #41031 · HEAD [f55eca2](https://github.com/apache/superset/commit/f55eca20aadeaa09753b49bab0e91205544041fe) > **comment** — Token plumbing is clean and genuinely opt-in, but two changes alter _default_ appearance outside the "no visual change without tokens" claim, and the new mapping logic ships no tests. CI is green; no human review yet. > > Context: adds 5 optional `sqlLabGrid*` theme tokens that flow ResultSet → FilterableTable → GridTable → `ThemedAgGridReact.withParams()` (sound reuse of existing infra), de-`!important`s the SQL Lab tabs, and adds `overflow: hidden` to all modals. > > ### 🔴 Functional > * **`packages/superset-ui-core/src/components/Modal/Modal.tsx:93`** — `overflow: hidden` on `.ant-modal-content` is global (every modal built on `StyledModal`, incl. `FormModal`) and not token-gated. It clips popups that render _inside_ the modal content via `getPopupContainer={t => t.closest('.ant-modal-content')}` — option lists in role/group pickers get cut off. codeant flagged this as Major and the diff doesn't address it. Scope the radius clipping to a wrapper that doesn't host popup containers instead of hiding overflow globally? (test: open a `Select` near a modal's bottom edge, assert the dropdown isn't clipped) > > ### 🟡 Should-fix > * **`SqlLab/components/RunQueryActionButton/index.tsx:83`** — this removes `min-width: auto` and the explicit `padding` (not just the `!important`) and adds `transition`/`margin-right`, so the Run button's default padding/width changes for everyone. The PR body says this file is just de-`!important`ed with `&&` "same visual weight" — but it's not `&&` here and the layout shifts. Intentional? If so add a before/after; if not, restore via `&&` specificity. > * **`SqlLab/components/ResultSet/index.tsx:217-240`** — the new `resultsGridThemeOverrides` mapping is the only uncovered patch code (codecov 56%, all 7 missing lines here). No test asserts a token maps to the right override key or that an empty config yields `undefined` (unchanged behavior). Add a Jest test per the regression-guard rule. > * **`SqlLab/components/ResultSet/index.tsx:218`, `components/FilterableTable/types.ts:34`, `components/GridTable/types.ts:63`** — `Record<string, any>` violates the no-`any` rule (already flagged by codeant/bito; non-blocking since CI is green). It mirrors the existing `ThemedAgGridReactProps.themeOverrides` type — reuse that type or define a typed `GridThemeOverrides` interface (`{ headerFontSize?: number; rowHeight?: number; oddRowBackgroundColor?: string; ... }`). > * **PR description** — lists a `selectOptionActiveOutline` token and a Select-hover change, but neither is in the diff (`theme/types.ts` adds only the 5 `sqlLabGrid*` tokens; no Select file is touched). Stale description or a missing file — reconcile before merge. > > ### 🔵 Nits > * `components/GridTable/index.tsx:187-188` — `rowHeight`/`headerHeight` are now set in three places: `gridOptions`, direct props, and `themeOverrides` theme params. The direct props duplicate `gridOptions`; drop them to avoid the AG Grid gridOptions-vs-props overlap. > > ### 🙌 Praise > * Good reuse of the existing `ThemedAgGridReact` `themeOverrides` → `withParams` pipeline — the tokens are genuinely opt-in (no config → `undefined` → identical to master). > > _Reviewed by EnxDev's Review Agent — @EnxDev · HEAD [f55eca2](https://github.com/apache/superset/commit/f55eca20aadeaa09753b49bab0e91205544041fe)._ @EnxDev Thanks for the thorough review! All items addressed in the latest push: - 🔴 Modal overflow: hidden — Reverted entirely, zero diff vs master now. - 🟡 RunQueryActionButton — Reverted entirely, zero diff vs master. - 🟡 Missing tests — Extracted mapping logic into buildResultsGridThemeOverrides.ts with 9 dedicated unit tests. - 🟡 Record<string, any> — Replaced with a typed GridThemeOverrides interface. - 🟡 Stale PR description — Updated to reflect actual diff (removed Select/Modal references). - 🔵 Redundant props — Removed duplicate rowHeight/headerHeight direct props; gridOptions is the single source now. Also renamed tokens from sqlLabGrid* → resultsGrid* per @villebro's feedback on naming generality. -- 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]
