bito-code-review[bot] commented on code in PR #40680:
URL: https://github.com/apache/superset/pull/40680#discussion_r3385001051
##########
superset-frontend/src/components/GridTable/Header.test.tsx:
##########
@@ -38,58 +38,147 @@ jest.mock('@superset-ui/core/components/Icons', () => {
};
});
-class MockApi extends EventTarget {
+class MockColumn {
+ private colListeners = new Map<string, Set<Function>>();
+
+ sortValue: string | null = 'asc';
+
+ sortIndexValue: number | null = null;
+
+ getColId() {
+ return '123';
+ }
+
+ isPinnedLeft() {
+ return true;
+ }
+
+ isPinnedRight() {
+ return false;
+ }
+
+ isVisible() {
+ return true;
+ }
+
+ getSort() {
+ return this.sortValue;
+ }
+
+ getSortIndex() {
+ return this.sortIndexValue;
+ }
+
+ addEventListener(eventType: string, listener: Function) {
+ if (!this.colListeners.has(eventType)) {
+ this.colListeners.set(eventType, new Set());
+ }
+ this.colListeners.get(eventType)!.add(listener);
+ }
+
+ removeEventListener(eventType: string, listener: Function) {
+ this.colListeners.get(eventType)?.delete(listener);
+ }
+
+ triggerEvent(eventType: string) {
+ this.colListeners.get(eventType)?.forEach(listener => listener({}));
+ }
+}
+
+class MockOtherColumn extends MockColumn {
+ getColId() {
+ return 'other-col';
+ }
+}
+
+class MockApi {
+ mockColumn = new MockColumn();
+
+ otherColumn = new MockOtherColumn();
+
getAllDisplayedColumns() {
- return [];
+ return [this.mockColumn, this.otherColumn];
+ }
+
+ getColumns() {
+ return [this.mockColumn, this.otherColumn];
}
isDestroyed() {
return false;
}
}
+const mockApi = new MockApi();
+
const mockedProps = {
displayName: 'test column',
- setSort: jest.fn(),
+ progressSort: jest.fn(),
enableSorting: true,
- column: {
- getColId: () => '123',
- isPinnedLeft: () => true,
- isPinnedRight: () => false,
- getSort: () => 'asc',
- getSortIndex: () => null,
- } as any as Column,
- api: new MockApi() as any as GridApi,
+ column: mockApi.mockColumn as any as Column,
+ api: mockApi as any as GridApi,
};
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test coverage gap</b></div>
<div id="fix">
All existing tests use `enableSorting: true` in mockedProps (line 117).
Header.tsx lines 138-144 only attach click handler when `enableSorting` is
truthy. Missing test coverage for the disabled sorting scenario per rule
[6262]: Assert Behavior Logic in Tests.
</div>
</div>
<small><i>Code Review Run #7e0f78</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/components/GridTable/Header.tsx:
##########
@@ -37,11 +32,9 @@ interface Params {
displayName: string;
column: Column;
api: GridApi;
- setSort: (sort: string | null, multiSort: boolean) => void;
+ progressSort: (multiSort?: boolean) => void;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-755: Missing Required Callback</b></div>
<div id="fix">
The `progressSort` prop is required by `Header` (line 35) but never provided
by `GridTable` in `index.tsx`. When ag-grid instantiates the custom header
component via `components.agColumnHeader`, it only passes its standard params
(column, api, displayName, enableSorting). Custom props cannot be injected
without a wrapper. The test file works because it renders `<Header
{...mockedProps} />` directly, bypassing ag-grid. At runtime, `progressSort`
will be undefined, causing `TypeError: progressSort is not a function` on line
100. (See also: [CWE-755](https://cwe.mitre.org/data/definitions/755.html))
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset-frontend/src/components/GridTable/index.tsx
+++ b/superset-frontend/src/components/GridTable/index.tsx
@@ -28,8 +28,18 @@ import { PIVOT_COL_ID, GridSize } from './constants';
import { Header } from './Header';
import type { TableProps } from './types';
+// Wrapper to inject progressSort into ag-grid's custom header component
+const createHeaderComponent = (progressSort: (multiSort?: boolean) =>
void) => {
+ const WrappedHeader: React.FC<any> = (props) => {
+ return <Header {...props} progressSort={progressSort} />;
+ };
+ return WrappedHeader;
+};
+
const gridComponents = {
- agColumnHeader: Header,
+ agColumnHeader: createHeaderComponent(() => {
+ // TODO: Implement multi-sort progress callback
+ }),
};
```
</div>
</details>
</div>
<small><i>Code Review Run #7e0f78</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
