geido commented on code in PR #42112:
URL: https://github.com/apache/superset/pull/42112#discussion_r3970033104
##########
superset-frontend/packages/superset-ui-core/src/chart/models/ChartProps.ts:
##########
@@ -112,7 +112,7 @@ export interface ChartPropsConfig {
/** is the chart refreshing its contents */
isRefreshing?: boolean;
/** chart ref */
- inputRef?: RefObject<any>;
+ inputRef?: RefObject<any | null>;
Review Comment:
`any | null` collapses to plain `any`, so this one is a no-op, along with
the other ~25 `RefObject<any | null>` changes in the PR (SuperChart,
labelUtils, ColumnOption/MetricOption, the `filters/*` and
`chartCustomizations/*` types, SliceHeaderControls, and `useRef<any | null>` in
Vertical.tsx).
Not a correctness problem, but reverting them would cut a good chunk of
noise and make the real `RefObject<HTMLDivElement | null>` changes much easier
to pick out.
##########
superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx:
##########
@@ -685,10 +685,10 @@ const Select = forwardRef(
const isLoading = loading ?? false;
const popupRender = (
- originNode: ReactElement & { ref?: RefObject<HTMLElement> },
+ originNode: ReactElement & { ref?: RefObject<HTMLElement | null> },
) =>
dropDownRenderHelper(
- originNode,
+ originNode as Parameters<typeof dropDownRenderHelper>[0],
Review Comment:
Minor: `utils.tsx` now types this parameter as
`ReactElement<FlattenOptionsProps>`, and then the cast here throws that away.
If you widen the local `popupRender` param to the same type, the cast can go
entirely. Same thing in `AsyncSelect.tsx`.
##########
superset-frontend/packages/superset-ui-core/src/components/CronPicker/CronPicker.test.tsx:
##########
@@ -28,13 +28,14 @@ test('Should send correct props to ReactCronPicker', () => {
myCustomProp: 'myCustomProp',
};
render(<CronPicker {...(props as any)} />);
- expect(spy).toHaveBeenCalledWith(
+ // React 19 calls function components with a single props argument, so assert
+ // on the props rather than the whole call signature.
+ expect(spy.mock.calls.at(-1)?.[0]).toEqual(
Review Comment:
`toHaveBeenCalledWith` matched *any* call, whereas `at(-1)` only matches the
last one, which makes this order-sensitive under StrictMode double-rendering.
You used `mock.calls.map(call => call[0])` + `toContainEqual` in
DashboardPage.test and PropertiesModal.test, which preserves the original
semantics. Worth using that shape here too (and in Tab.test /
ThemedAgGridReact.test) so they all read the same way.
--
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]
