codeant-ai-for-open-source[bot] commented on code in PR #36720:
URL: https://github.com/apache/superset/pull/36720#discussion_r2629176985
##########
superset-frontend/src/dashboard/components/filterscope/renderFilterFieldTreeNodes.tsx:
##########
@@ -16,23 +16,42 @@
* specific language governing permissions and limitations
* under the License.
*/
+import { ReactNode } from 'react';
import FilterFieldItem from './FilterFieldItem';
-export default function renderFilterFieldTreeNodes({ nodes, activeKey }) {
+export interface FilterScopeTreeNode {
+ value: string | number;
+ label: string | ReactNode;
+ type?: string;
+ children?: FilterScopeTreeNode[];
+}
+
+interface RenderFilterFieldTreeNodesParams {
+ nodes: FilterScopeTreeNode[] | null;
+ activeKey?: string | null;
+}
+
+export default function renderFilterFieldTreeNodes({
+ nodes,
+ activeKey,
+}: RenderFilterFieldTreeNodesParams): FilterScopeTreeNode[] {
if (!nodes) {
Review Comment:
**Suggestion:** The function assumes that when `nodes` is non-null it has at
least one element, so calling `nodes[0]` when an empty array is passed will
make `root` undefined and cause a runtime error when accessing `root.children`
or `root.label`; add a length check so empty arrays are handled safely. [null
pointer]
**Severity Level:** Minor ⚠️
```suggestion
if (!nodes || nodes.length === 0) {
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The PR's current code only guards against nodes being null, not against an
empty array.
If nodes === [], root becomes undefined and subsequent access to
root.children or root.label will throw at runtime.
The proposed check (nodes.length === 0) is a small, correct, and defensive
fix that prevents a real runtime error.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/filterscope/renderFilterFieldTreeNodes.tsx
**Line:** 38:38
**Comment:**
*Null Pointer: The function assumes that when `nodes` is non-null it
has at least one element, so calling `nodes[0]` when an empty array is passed
will make `root` undefined and cause a runtime error when accessing
`root.children` or `root.label`; add a length check so empty arrays are handled
safely.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/dashboard/components/filterscope/FilterFieldTree.tsx:
##########
@@ -16,54 +16,43 @@
* specific language governing permissions and limitations
* under the License.
*/
-import PropTypes from 'prop-types';
-import CheckboxTree from 'react-checkbox-tree';
-import { filterScopeSelectorTreeNodePropShape } from
'src/dashboard/util/propShapes';
+import CheckboxTree, { Node } from 'react-checkbox-tree';
import treeIcons from './treeIcons';
-import renderFilterFieldTreeNodes from './renderFilterFieldTreeNodes';
+import renderFilterFieldTreeNodes, {
+ FilterScopeTreeNode,
+} from './renderFilterFieldTreeNodes';
-const propTypes = {
- activeKey: PropTypes.string,
- nodes: PropTypes.arrayOf(filterScopeSelectorTreeNodePropShape).isRequired,
- checked: PropTypes.arrayOf(
- PropTypes.oneOfType([PropTypes.number, PropTypes.string]),
- ).isRequired,
- expanded: PropTypes.arrayOf(
- PropTypes.oneOfType([PropTypes.number, PropTypes.string]),
- ).isRequired,
- onCheck: PropTypes.func.isRequired,
- onExpand: PropTypes.func.isRequired,
- onClick: PropTypes.func.isRequired,
-};
-
-const defaultProps = {
- activeKey: null,
-};
+interface FilterFieldTreeProps {
+ activeKey?: string | null;
+ nodes: FilterScopeTreeNode[];
+ checked: (string | number)[];
+ expanded: (string | number)[];
+ onCheck: (checked: string[]) => void;
+ onExpand: (expanded: string[]) => void;
+ onClick: (node: { value: string }) => void;
Review Comment:
**Suggestion:** The `onClick` callback is typed as receiving only `{ value:
string }`, but `react-checkbox-tree` actually passes a full `Node` whose
`value` may be a string or a number; this mismatch can lead to callers making
invalid assumptions about the payload shape and value type. [type error]
**Severity Level:** Minor ⚠️
```suggestion
onClick: (node: Node) => void;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
CheckboxTree's onClick delivers the full Node object (imported from
react-checkbox-tree) and Node.value can be string or number. Typing the prop as
Node is more accurate and prevents callers from assuming a narrower payload
shape. Unlike the checked/expanded cases, there is no local coercion that
guarantees value is a string before onClick is invoked, so widening the type to
Node better reflects runtime.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/filterscope/FilterFieldTree.tsx
**Line:** 32:32
**Comment:**
*Type Error: The `onClick` callback is typed as receiving only `{
value: string }`, but `react-checkbox-tree` actually passes a full `Node` whose
`value` may be a string or a number; this mismatch can lead to callers making
invalid assumptions about the payload shape and value type.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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]
