codeant-ai-for-open-source[bot] commented on code in PR #42330: URL: https://github.com/apache/superset/pull/42330#discussion_r3636172640
########## superset-frontend/src/filters/components/CustomDateFilter/CustomDateFilterPlugin.tsx: ########## @@ -0,0 +1,185 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { useCallback, useEffect, useState } from 'react'; +import { JsonObject } from '@superset-ui/core'; +import { styled } from '@apache-superset/core/theme'; +import { DatePicker } from '@superset-ui/core/components'; +import { FilterPluginStyle } from '../common'; +import { PluginFilterCustomDateFilterQueryFormData, DEFAULT_FORM_DATA } from './types'; + +interface CustomDateFilterPluginProps { + data: { [key: string]: any }[]; + formData: PluginFilterCustomDateFilterQueryFormData; + width: number; + height: number; + filterState: { value?: any[] }; + setDataMask: (arg: JsonObject) => void; + setHoveredFilter: (arg: string) => void; + unsetHoveredFilter: () => void; + setFocusedFilter: (arg: string) => void; + unsetFocusedFilter: () => void; + setFilterActive: (arg: boolean) => void; + inputRef: React.RefObject<HTMLInputElement>; + isRefreshing: boolean; + appSection: string; +} + +const StyledDatePicker = styled.div` + display: flex; + align-items: center; + justify-content: center; + height: 100%; + padding: 8px; +`; + +export default function CustomDateFilterPlugin( + props: CustomDateFilterPluginProps, +) { + const { + formData, + width, + height, + filterState, + setDataMask, + setHoveredFilter, + unsetHoveredFilter, + setFocusedFilter, + unsetFocusedFilter, + isRefreshing, + } = props; + + const { + controlType = 'date', + granularitySqla, + } = { ...DEFAULT_FORM_DATA, ...formData }; + + const [dateValue, setDateValue] = useState<any>( + filterState.value || null, + ); + + const handleChange = useCallback( + (date: any, _dateString: string | [string, string]) => { + setDateValue(date); + + let filters: any[] = []; + if (date) { + if (controlType === 'daterange' && Array.isArray(date)) { + filters = [ + { + col: granularitySqla, + op: '>=', + val: date[0].toISOString(), + }, + { + col: granularitySqla, + op: '<=', + val: date[1].toISOString(), + }, + ]; + } else if (date instanceof Date) { Review Comment: **Suggestion:** The filter objects are created even when `granularitySqla` is empty/undefined, producing clauses with an undefined column name that can cause backend query errors. Guard filter creation on a valid time column (or make the control required) before calling `setDataMask`. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Native filter can emit filters with undefined column. - ⚠️ Downstream charts may error when such filter applied. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. `CustomDateFilterPlugin` derives `granularitySqla` from form data at `CustomDateFilterPlugin.tsx:68-71` via `{ ...DEFAULT_FORM_DATA, ...formData }`; `buildQuery.ts` explicitly guards `if (dateRange && dateRange.length === 2 && granularitySqla)` at `buildQuery.ts:36`, which shows `granularitySqla` can legitimately be empty/undefined in some configurations. 2. Configure a “Custom Date Filter” chart in Explore without selecting a time column in its control panel (the field that populates `formData.granularitySqla` referenced in `buildQuery.ts:31`); in this case `granularitySqla` remains `undefined` in the props passed into `CustomDateFilterPlugin` at `CustomDateFilterPlugin.tsx:55-66`. 3. On the rendered chart, choose the “Date range” control type and pick a start and end date so that `controlType === 'daterange'` and `date` is a two-element array when `handleChange` runs at `CustomDateFilterPlugin.tsx:77-117`. 4. Inside `handleChange`, the condition at `CustomDateFilterPlugin.tsx:83` passes and the code constructs `filters` at `CustomDateFilterPlugin.tsx:84-95` using `col: granularitySqla`, which is `undefined`; `setDataMask` is then called at `CustomDateFilterPlugin.tsx:107-115` with `extraFormData.filters` containing two clauses whose `col` is `undefined`, so any downstream consumer that converts these filter clauses into a query (e.g., native filter application to charts) will receive an invalid column name and may raise server-side query errors or ignore the filter unexpectedly. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=59c3f8ec224a45e0a65013026de3b70b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=59c3f8ec224a45e0a65013026de3b70b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/filters/components/CustomDateFilter/CustomDateFilterPlugin.tsx **Line:** 84:95 **Comment:** *Possible Bug: The filter objects are created even when `granularitySqla` is empty/undefined, producing clauses with an undefined column name that can cause backend query errors. Guard filter creation on a valid time column (or make the control required) before calling `setDataMask`. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42330&comment_hash=ebbeae4e1d422065e11ae00a3fa80fd10f9f360dca55190c33c93ce0e0a8118a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42330&comment_hash=ebbeae4e1d422065e11ae00a3fa80fd10f9f360dca55190c33c93ce0e0a8118a&reaction=dislike'>👎</a> ########## superset-frontend/src/filters/components/CustomDateFilter/buildQuery.ts: ########## @@ -0,0 +1,59 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { + buildQueryContext, + QueryObject, + QueryObjectFilterClause, + BuildQuery, +} from '@superset-ui/core'; +import { DEFAULT_FORM_DATA, PluginFilterCustomDateFilterQueryFormData } from './types'; + +const buildQuery: BuildQuery<PluginFilterCustomDateFilterQueryFormData> = ( + formData: PluginFilterCustomDateFilterQueryFormData, + _options, +) => { + const { dateRange, granularitySqla } = { ...DEFAULT_FORM_DATA, ...formData }; + return buildQueryContext(formData, baseQueryObject => { + const { filters = [] } = baseQueryObject; + const extraFilters: QueryObjectFilterClause[] = []; + + if (dateRange && dateRange.length === 2 && granularitySqla) { + extraFilters.push({ + col: granularitySqla, + op: '>=', + val: dateRange[0], + }); + extraFilters.push({ + col: granularitySqla, + op: '<=', + val: dateRange[1], + }); + } Review Comment: **Suggestion:** Range filters are added whenever `dateRange` has two values, regardless of the selected control type; if users switch from range to single-date mode, stale `dateRange` values can still be applied unintentionally. Gate this block by `controlType === 'daterange'` to avoid leaking old state into queries. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Chart query can use stale dateRange after mode switch. - ⚠️ Explore results may not match visible filter control. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. The `buildQuery` implementation for this viz retrieves `dateRange` and `granularitySqla` from form data at `buildQuery.ts:31`, and later unconditionally applies range filters whenever `dateRange` has two entries and `granularitySqla` is truthy, as seen in `buildQuery.ts:36-46`. 2. In Explore for the “Custom Date Filter” visualization (registered via `superset-frontend/src/filters/components/CustomDateFilter/index.ts:25-26` and `superset-frontend/src/visualizations/presets/MainPreset.ts:94-95`), first configure the filter with `controlType === 'daterange'` in its control panel (defined in `superset-frontend/src/filters/components/CustomDateFilter/controlPanel.ts`) and select a valid date range, which populates `formData.dateRange` with two values. 3. Without clearing or resetting the form, change the control type to a single-date mode (`controlType === 'date'` or `'datetime'`); the UI hides the range input, but `formData.dateRange` retains the previously selected two-element array, since `buildQuery.ts` does not clear it and only merges `DEFAULT_FORM_DATA` with the existing `formData` at `buildQuery.ts:31`. 4. Trigger a query for the filter chart; when `buildQuery` runs, the condition at `buildQuery.ts:36` (`dateRange && dateRange.length === 2 && granularitySqla`) still evaluates to true, so the old `dateRange` values are pushed into `extraFilters` at `buildQuery.ts:37-46`, meaning the backend query continues to be constrained by the stale range even though the UI indicates a non-range control type. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=62908b374b704d128a39981a3fb18236&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=62908b374b704d128a39981a3fb18236&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/filters/components/CustomDateFilter/buildQuery.ts **Line:** 31:47 **Comment:** *Logic Error: Range filters are added whenever `dateRange` has two values, regardless of the selected control type; if users switch from range to single-date mode, stale `dateRange` values can still be applied unintentionally. Gate this block by `controlType === 'daterange'` to avoid leaking old state into queries. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42330&comment_hash=1cee6b86079f7c7f6a05ac32d6c3e19258f1d54a4de3a8032984d73acc68e86b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42330&comment_hash=1cee6b86079f7c7f6a05ac32d6c3e19258f1d54a4de3a8032984d73acc68e86b&reaction=dislike'>👎</a> ########## superset-frontend/src/filters/components/CustomDateFilter/CustomDateFilterPlugin.tsx: ########## @@ -0,0 +1,185 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { useCallback, useEffect, useState } from 'react'; +import { JsonObject } from '@superset-ui/core'; +import { styled } from '@apache-superset/core/theme'; +import { DatePicker } from '@superset-ui/core/components'; +import { FilterPluginStyle } from '../common'; +import { PluginFilterCustomDateFilterQueryFormData, DEFAULT_FORM_DATA } from './types'; + +interface CustomDateFilterPluginProps { + data: { [key: string]: any }[]; + formData: PluginFilterCustomDateFilterQueryFormData; + width: number; + height: number; + filterState: { value?: any[] }; + setDataMask: (arg: JsonObject) => void; + setHoveredFilter: (arg: string) => void; + unsetHoveredFilter: () => void; + setFocusedFilter: (arg: string) => void; + unsetFocusedFilter: () => void; + setFilterActive: (arg: boolean) => void; + inputRef: React.RefObject<HTMLInputElement>; + isRefreshing: boolean; + appSection: string; +} + +const StyledDatePicker = styled.div` + display: flex; + align-items: center; + justify-content: center; + height: 100%; + padding: 8px; +`; + +export default function CustomDateFilterPlugin( + props: CustomDateFilterPluginProps, +) { + const { + formData, + width, + height, + filterState, + setDataMask, + setHoveredFilter, + unsetHoveredFilter, + setFocusedFilter, + unsetFocusedFilter, + isRefreshing, + } = props; + + const { + controlType = 'date', + granularitySqla, + } = { ...DEFAULT_FORM_DATA, ...formData }; + + const [dateValue, setDateValue] = useState<any>( + filterState.value || null, + ); + + const handleChange = useCallback( + (date: any, _dateString: string | [string, string]) => { + setDateValue(date); + + let filters: any[] = []; + if (date) { + if (controlType === 'daterange' && Array.isArray(date)) { + filters = [ + { + col: granularitySqla, + op: '>=', + val: date[0].toISOString(), + }, + { + col: granularitySqla, + op: '<=', + val: date[1].toISOString(), + }, + ]; + } else if (date instanceof Date) { + filters = [ + { + col: granularitySqla, + op: '=', + val: date.toISOString(), + }, + ]; + } + } Review Comment: **Suggestion:** The single-date branch checks `date instanceof Date`, but the Superset/AntD date picker returns Dayjs objects, not native `Date`; this condition will fail and no filter will be emitted for `date`/`datetime` selections. Use a value-type check compatible with the picker output and convert it to ISO before building filters. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Single-date custom date filters never apply any filter. - ⚠️ Dashboards show selection but dependent charts stay unfiltered. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Register and enable the `FilterCustomDateFilterPlugin` visualization in the frontend; this is wired through `superset-frontend/src/filters/components/CustomDateFilter/index.ts:25-26` and added to the main preset in `superset-frontend/src/visualizations/presets/MainPreset.ts:94-95`. 2. In Explore, create a new chart using the “Custom Date Filter” viz and ensure the control panel uses the default `controlType` (the default is `'date'` as set in `CustomDateFilterPlugin.tsx:68-71` when merging `DEFAULT_FORM_DATA` and `formData`). 3. Render the chart so the React component `CustomDateFilterPlugin` at `CustomDateFilterPlugin.tsx:52-65` mounts; the single-date branch renders `<DatePicker>` (not `RangePicker`) at `CustomDateFilterPlugin.tsx:163-172` with its `onChange` bound to `handleChange` defined at `CustomDateFilterPlugin.tsx:77-117`. 4. Use the UI to pick a single date (or datetime, with `controlType === 'datetime'`); the `DatePicker` from `@superset-ui/core/components` passes a Dayjs-like object (not a native `Date`) into `handleChange` at `CustomDateFilterPlugin.tsx:78`, so: - `controlType !== 'daterange'` makes the range branch at `CustomDateFilterPlugin.tsx:83-95` skip. - `date instanceof Date` at `CustomDateFilterPlugin.tsx:96` evaluates to `false` for a Dayjs object, so the single-date branch never runs and `filters` remains the empty array initialized at `CustomDateFilterPlugin.tsx:81`. - `setDataMask` is then called at `CustomDateFilterPlugin.tsx:107-115` with `extraFormData.filters` empty but `filterState.value` set to `[date]`, so dashboards show a selected date while no backend filter is ever applied. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=121abd8aeca44aa292e0bd6c5d3f142a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=121abd8aeca44aa292e0bd6c5d3f142a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/filters/components/CustomDateFilter/CustomDateFilterPlugin.tsx **Line:** 96:104 **Comment:** *Logic Error: The single-date branch checks `date instanceof Date`, but the Superset/AntD date picker returns Dayjs objects, not native `Date`; this condition will fail and no filter will be emitted for `date`/`datetime` selections. Use a value-type check compatible with the picker output and convert it to ISO before building filters. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42330&comment_hash=8217c196669ffbdf36e33b03b7e6ec5be7cd7ef789695340f3cf372c335d0aac&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42330&comment_hash=8217c196669ffbdf36e33b03b7e6ec5be7cd7ef789695340f3cf372c335d0aac&reaction=dislike'>👎</a> -- 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]
