rusackas commented on code in PR #37625:
URL: https://github.com/apache/superset/pull/37625#discussion_r2770109487
##########
superset-frontend/src/dashboard/actions/hydrate.ts:
##########
@@ -146,16 +205,19 @@ export const hydrateDashboard =
{
chartId: slice.slice_id,
},
- (newSlicesContainer.parents || []).slice(),
+ (newSlicesContainer!.parents || []).slice(),
);
const count = (slicesFromExploreCount.get(slice.slice_id) ?? 0) + 1;
chartHolder.id = `${CHART_TYPE}-explore-${slice.slice_id}-${count}`;
slicesFromExploreCount.set(slice.slice_id, count);
layout[chartHolder.id] = chartHolder;
- newSlicesContainer.children.push(chartHolder.id);
- chartIdToLayoutId[chartHolder.meta.chartId] = chartHolder.id;
+ newSlicesContainer!.children.push(chartHolder.id);
+ const holderChartId = chartHolder.meta.chartId;
+ if (holderChartId !== undefined) {
+ chartIdToLayoutId[holderChartId] = chartHolder.id;
+ }
Review Comment:
Good observation. The `newSlicesContainer\!` assertion is safe because it's
only reached inside the `if (\!chartIdToLayoutId[key] && layout[parentId])`
block where `newSlicesContainer` is always assigned (either from the
width-check branch above or from a previous iteration). The assertion just
documents this invariant to TypeScript. Moving the lines inside the conditional
would require restructuring the control flow since `newSlicesContainer`
persists across iterations of the `forEach`. This is a pre-existing pattern
from the JS code — refactoring the loop structure is beyond migration scope.
##########
superset-frontend/src/dashboard/components/dnd/DragDroppable.test.tsx:
##########
@@ -102,23 +105,23 @@ describe('DragDroppable', () => {
});
test('should call its child function with "dropIndicatorProps" dependent on
editMode and isDraggingOver', () => {
- const renderChild = jest.fn(provided => (
+ const renderChild = jest.fn((provided: Record<string, unknown>) => (
<div data-test="child-content" {...provided}>
Test Content
</div>
));
// Create a mock component with the dropIndicator state already set
class MockDragDroppable extends DragDroppable {
- constructor(props) {
- super(props);
- this.state = { dropIndicator: true };
+ constructor(mockProps: ConstructorParameters<typeof DragDroppable>[0]) {
+ super(mockProps);
+ this.state = { dropIndicator: 'DROP_TOP' as any };
}
}
render(
<MockDragDroppable
- {...props}
+ {...(props as any)}
Review Comment:
Agreed in principle, but this test intentionally passes a minimal/partial
props object to verify the component's behavior with incomplete data. Defining
a strict test props type would require either duplicating the full
DragDroppable props interface or importing it and constructing a complete
object — both of which are more effort than value for a test that's
specifically verifying render behavior with partial props. The `as any` here is
the standard pattern for partial mock props in test files across the codebase.
--
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]
