bito-code-review[bot] commented on code in PR #43275: URL: https://github.com/apache/superset/pull/43275#discussion_r3799931438
########## superset-frontend/src/filters/components/CustomControls/index.ts: ########## @@ -0,0 +1,47 @@ +/** + * 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 { t } from '@apache-superset/core/translation'; +import { Behavior, ChartMetadata, ChartPlugin } from '@superset-ui/core'; +import buildQuery from './buildQuery'; +import controlPanel from './controlPanel'; +import transformProps from './transformProps'; +import thumbnail from '../Select/images/thumbnail.png'; Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incorrect thumbnail path reference</b></div> <div id="fix"> The thumbnail import references `../Select/images/thumbnail.png` (Select filter's assets) instead of a local `./images/thumbnail.png`. Each filter plugin should have its own thumbnail following the established pattern (verified in Select, DateTimeFilter, Range). The `CustomControls` directory lacks an `images/` subdirectory entirely. </div> </div> <small><i>Code Review Run #e41105</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/filters/components/DateTimeFilter/types.ts: ########## @@ -0,0 +1,40 @@ +/** + * 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 { Behavior, FilterState, QueryFormData } from '@superset-ui/core'; +import { RefObject } from 'react'; +import { PluginFilterHooks, PluginFilterStylesProps } from '../types'; +import { FilterBarOrientation } from '../../../dashboard/types'; Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Inconsistent import path convention</b></div> <div id="fix"> Replace the relative path `../../../dashboard/types` with the project-wide `src/` alias `src/dashboard/types` to match the import convention used in the sibling filter type files (`Select/types.ts`, `CustomControls/types.ts`). </div> </div> <small><i>Code Review Run #e41105</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/filters/components/CustomControls/types.ts: ########## @@ -0,0 +1,66 @@ +/** + * 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 { + QueryFormData, + TimeseriesDataRecord, + DataMask, +} from '@superset-ui/core'; +import { SupersetTheme } from '@apache-superset/core/theme'; +import { FilterBarOrientation } from 'src/dashboard/types'; + +export interface CustomControlsStylesProps { + height: number; + width: number; +} + +export type ControlType = 'Dropdown' | 'Radio' | 'Checkbox' | 'TextBox'; + +export interface PluginFilterCustomControlsQueryFormData extends QueryFormData { + controlType: ControlType; + filterColumn?: string | { label?: string; column_name?: string; sqlExpression?: string }; + orientation?: 'vertical' | 'horizontal'; + includeAllOption?: boolean; + multiSelect?: boolean; + enableEmptyFilter?: boolean; + controlValues?: Record<string, unknown>; + inCanvas?: boolean; +} + +export interface ExtendedTheme extends SupersetTheme {} Review Comment: <div> <div id="suggestion"> <div id="issue"><b>SEMANTIC_DUPLICATION: Redundant type alias</b></div> <div id="fix"> The `ExtendedTheme` interface is semantically duplicate — it only re-exports `SupersetTheme` with no additions. Either remove it and use `SupersetTheme` directly on line 63, or use the existing type directly to reduce indirection. </div> </div> <small><i>Code Review Run #e41105</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/filters/components/DateTimeFilter/controlPanel.ts: ########## @@ -0,0 +1,67 @@ +/** + * 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 { Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Consolidate duplicate control panel config imports</b></div> <div id="fix"> Identified syntactic code duplication across filter component control panels. Files CustomControls/controlPanel.ts (lines 19-44), DateTimeFilter/controlPanel.ts (lines 19-44), Time/controlPanel.ts (lines 19-44), DateTimeFilter/controlPanel.ts (lines 23-53), Range/controlPanel.ts (lines 24-54), and Select/controlPanel.ts (lines 68-95) contain duplicate import statements and config structures. Consider extracting shared configuration into a common utility module to improve maintainability. </div> </div> <small><i>Code Review Run #e41105</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/filters/components/DateTimeFilter/DateTimeFilterPlugin.tsx: ########## @@ -0,0 +1,1028 @@ +/** + * 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, useRef, useState, useMemo } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { + NO_TIME_RANGE, + fetchTimeRange, + SEPARATOR, + JsonObject, +} from '@superset-ui/core'; +import { styled, useTheme } from '@apache-superset/core/theme'; +import { + RangePicker, + Button, + Divider, + AntdThemeProvider, + InfoTooltip, + Popover, +} from '@superset-ui/core/components'; +import { + CommonFrame, + CalendarFrame, + CurrentCalendarFrame, + CustomFrame, +} from 'src/explore/components/controls/DateFilterControl/components'; +import { DateFilterTestKey } from 'src/explore/components/controls/DateFilterControl/utils'; +import { FilterPluginStyle } from '../common'; +import { PluginFilterDateTimeProps } from './types'; +import { useLocale } from 'src/hooks/useLocale'; +import dayjs from 'dayjs'; + +// Matches date strings returned by fetchTimeRange, e.g.: +// "2026-04-23 ≤ col < 2026-04-29" +// "2026-04-23 00:00:00 ≤ col < 2026-04-29 00:00:00" +const RESOLVED_DATE_RE = /(\d{4}-\d{2}-\d{2}(?:[T ]\d{2}:\d{2}:\d{2})?)/g; + +/* ------------------------------------------------------------------ */ +/* Frame → Tab mapping */ +/* ------------------------------------------------------------------ */ + +type TabKey = 'basic' | 'last' | 'previous' | 'current' | 'custom' | 'advanced'; + +const TAB_CONFIG: { key: TabKey; label: string }[] = [ + { key: 'basic', label: 'Basic' }, + { key: 'last', label: 'Last' }, + { key: 'previous', label: 'Previous' }, + { key: 'current', label: 'Current' }, + { key: 'custom', label: 'Custom' }, +]; + +/* ------------------------------------------------------------------ */ +/* Styled wrappers */ +/* ------------------------------------------------------------------ */ + +const DateTimeFilterStyles = styled(FilterPluginStyle)` + display: flex; + align-items: center; + overflow-x: visible; +`; + +const ControlContainer = styled.div<{ + validateStatus?: 'error' | 'warning' | 'info'; +}>` + display: flex; + height: 100%; + max-width: 100%; + width: 100%; + + & > .ant-picker { + width: 100%; + flex: 1; + } +`; + +const PopoverContent = styled.div` + width: 600px; + max-width: 90vw; + + .tab-nav { + display: flex; + border-bottom: 1px solid ${({ theme }) => theme.colorBorderSecondary}; + padding: 0 8px; + margin-bottom: 0; + } + + .tab-nav-item { + padding: 6px 10px; + cursor: pointer; + font-size: 11px; + font-weight: 500; + letter-spacing: 0.02em; + color: ${({ theme }) => theme.colorTextSecondary}; + border-bottom: 2px solid transparent; + margin-bottom: -1px; + transition: all 0.2s; + + &:hover { + color: ${({ theme }) => theme.colorPrimary}; + } + + &.active { + color: ${({ theme }) => theme.colorPrimary}; + border-bottom-color: ${({ theme }) => theme.colorPrimary}; + } + } + + .tab-body { + padding: 8px 16px; + min-height: 100px; + + .section-title { + font-weight: 600; + font-size: 13px; + line-height: 20px; + margin-bottom: 6px; + letter-spacing: -0.01em; + } + + .control-label { + font-size: 11px; + font-weight: 500; + color: ${({ theme }) => theme.colorTextSecondary}; + margin-bottom: 6px; + text-transform: uppercase; + letter-spacing: 0.03em; + } + + .ant-input { + background: ${({ theme }) => theme.colorBgContainer} !important; + border: 1px solid ${({ theme }) => theme.colorBorder} !important; + color: ${({ theme }) => theme.colorText} !important; + padding: 6px 12px; + font-size: 12px; + border-radius: 4px; + + &:focus { + border-color: ${({ theme }) => theme.colorPrimary} !important; + box-shadow: 0 0 0 2px ${({ theme }) => theme.colorPrimary}22 !important; + } + + &::placeholder { + color: ${({ theme }) => + theme.colorTextPlaceholder || theme.colorTextQuaternary} !important; + } + } + + .ant-row { + margin-top: 8px; + } + .ant-picker { + padding: 4px 17px 4px; + border-radius: 4px; + } + .ant-divider-horizontal { + margin: 16px 0; + border-color: ${({ theme }) => theme.colorBorderSecondary}; + } + .control-anchor-to { + margin-top: 16px; + } + .control-anchor-to-datetime { + width: 217px; + } + } +`; + +const ActualTimeRange = styled.div` + font-size: 12px; + font-weight: 600; + color: ${({ theme }) => theme.colorText}; + padding: 4px 0; + font-family: ${({ theme }) => theme.fontFamilyCode}; + display: flex; + align-items: center; + flex-wrap: nowrap; + gap: 8px; + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; + + .label { + font-size: 11px; + font-weight: 500; + color: ${({ theme }) => theme.colorTextSecondary}; + text-transform: uppercase; + letter-spacing: 0.03em; + flex-shrink: 0; + } +`; + +/** + * Container that holds the inline calendar. + * + * The RangePicker renders two elements: + * 1. The <input> row — we collapse it to zero height so it's invisible. + * 2. The dropdown panel — we un-position it so it flows inline in the div. + * + * pointer-events on the input are set to none so clicks pass through to the + * calendar panels, which explicitly restore pointer-events. + */ +const InlineCalendarContainer = styled.div` + position: relative; + /* Enough height for two calendar months side by side */ + min-height: 290px; + margin-bottom: 12px; + + /* Collapse the picker INPUT element */ + .ant-picker { + position: absolute !important; + top: 0; + left: 0; + width: 0 !important; + height: 0 !important; + padding: 0 !important; + border: none !important; + overflow: hidden !important; + pointer-events: none !important; + opacity: 0 !important; + } + + /* Make the dropdown render statically inside this div */ + .ant-picker-dropdown { + position: static !important; + box-shadow: none !important; + padding: 0 !important; + background: transparent !important; + } + + .ant-picker-panel-container { + box-shadow: none !important; + border: none !important; + background: transparent !important; + } + + .ant-picker-header-view { + font-weight: 600; + font-size: 13px; + letter-spacing: -0.01em; + } + + .ant-picker-content th { + font-size: 11px; + color: ${({ theme }) => theme.colorTextDescription}; + font-weight: 500; + } + + /* Range selection colors — Preset Green */ + .ant-picker-cell-in-view.ant-picker-cell-in-range::before { + background: ${({ theme }) => theme.colorPrimary}22 !important; + } + .ant-picker-cell-in-view.ant-picker-cell-range-start .ant-picker-cell-inner, + .ant-picker-cell-in-view.ant-picker-cell-range-end .ant-picker-cell-inner { + background: ${({ theme }) => theme.colorPrimary} !important; + color: white !important; + } + .ant-picker-cell-in-view.ant-picker-cell-today + .ant-picker-cell-inner::before { + border-color: ${({ theme }) => theme.colorPrimary} !important; + } + + /* Restore click events on the actual calendar UI */ + .ant-picker-panel-container, + .ant-picker-panels, + .ant-picker-panel, + .ant-picker-body, + .ant-picker-content, + .ant-picker-header, + table, + th, + td { + pointer-events: auto !important; + } +`; + +const StatusTag = styled.span` + background: ${({ theme }) => theme.colorSuccessBg}; + color: ${({ theme }) => theme.colorSuccess}; + font-size: 10px; + font-weight: 700; + padding: 2px 6px; + border-radius: 10px; + text-transform: uppercase; + letter-spacing: 0.05em; + margin-right: 8px; + display: inline-flex; + align-items: center; + gap: 4px; + + &::before { + content: ''; + width: 6px; + height: 6px; + background: ${({ theme }) => theme.colorSuccess}; + border-radius: 50%; + } +`; + +const InputWrapper = styled.div` + position: relative; + width: 100%; + + .clear-icon { + position: absolute; + right: 8px; + top: 50%; + transform: translateY(-50%); + cursor: pointer; + color: ${({ theme }) => + theme.colorTextDescription || theme.colorTextTertiary}; + font-size: 12px; + transition: color 0.2s; + + &:hover { + color: ${({ theme }) => theme.colorText}; + } + } +`; + +/* ------------------------------------------------------------------ */ +/* Component */ +/* ------------------------------------------------------------------ */ + +export default function DateTimeFilterPlugin(props: PluginFilterDateTimeProps) { + const theme = useTheme(); + const { + setDataMask, + setHoveredFilter, + unsetHoveredFilter, + setFocusedFilter, + unsetFocusedFilter, + setFilterActive, + width, + height, + filterState, + inputRef, + isOverflowingFilterBar = false, + formData, + } = props; + + const col: string = useMemo(() => { + const jsonFormData = formData as JsonObject | undefined; + const rawCol = + jsonFormData?.groupby || + jsonFormData?.column || + (jsonFormData?.targets as JsonObject[] | undefined)?.[0]?.column?.name || + jsonFormData?.columnName; + if (typeof rawCol === 'string') return rawCol.trim(); + if (Array.isArray(rawCol) && rawCol.length > 0) return String(rawCol[0]).trim(); + if (rawCol && typeof rawCol === 'object') { + const colObj = rawCol as Record<string, unknown>; + return String( + colObj.label || colObj.column_name || colObj.sqlExpression || '', + ).trim(); + } + return ''; + }, [formData]); + + // ---- State ---- + const [show, setShow] = useState(false); + const [timeRangeValue, setTimeRangeValue] = useState<string>(NO_TIME_RANGE); + const [triggerDates, setTriggerDates] = useState< + [dayjs.Dayjs | null, dayjs.Dayjs | null] + >([null, null]); + const [evalResponse, setEvalResponse] = useState<string>(''); + const [validTimeRange, setValidTimeRange] = useState(true); + const [activeTab, setActiveTab] = useState<TabKey>('basic'); + // Bump to re-mount the inline RangePicker after the popover finishes animating in + const [calendarKey, setCalendarKey] = useState(0); + const [defaultPickerValue, setDefaultPickerValue] = useState< + [dayjs.Dayjs, dayjs.Dayjs] | undefined + >(undefined); + + const datePickerLocale = useLocale(); + const calendarContainerRef = useRef<HTMLDivElement>(null); + + // Parse since/until for the inline calendar value + const [since, until] = useMemo(() => { + if ( + timeRangeValue && + timeRangeValue !== NO_TIME_RANGE && + timeRangeValue.includes(SEPARATOR) + ) { + const parts = timeRangeValue.split(SEPARATOR); + return [parts[0]?.trim() || '', parts[1]?.trim() || '']; + } + return ['', '']; + }, [timeRangeValue]); + + const calValue: [dayjs.Dayjs | null, dayjs.Dayjs | null] = useMemo(() => { + // If we have a successful resolved range string (e.g. "2026-04-23 <= col < 2026-04-29"), + // use those dates to drive the calendar highlights even if the input is a formula. + if ( + evalResponse && + !evalResponse.includes('Invalid') && + evalResponse.includes('col') + ) { + const matches = [...evalResponse.matchAll(RESOLVED_DATE_RE)]; + if (matches.length >= 2) { + const start = dayjs(matches[0][1]); + const end = dayjs(matches[1][1]); + if (start.isValid() && end.isValid()) { + return [start, end]; + } + } + } + + // Fallback to direct parsing if it's a fixed date string + return [ + since && dayjs(since).isValid() ? dayjs(since) : null, + until && dayjs(until).isValid() ? dayjs(until) : null, + ]; + }, [since, until, evalResponse]); + + /* ---- Resolve filterState.value → trigger display --------------- */ + // Watch the dashboard's confirmed value and derive actual dates for the + // trigger RangePicker display. This survives re-mounts and page reloads. + useEffect(() => { + const value = (filterState.value as string) || NO_TIME_RANGE; + if (!value || value === NO_TIME_RANGE) { + setTriggerDates([null, null]); + return; + } + + // Synchronous path: value is already ISO date strings (e.g. "2026-04-01 : 2026-05-01") + if (value.includes(SEPARATOR)) { + const parts = value.split(SEPARATOR); + const s = parts[0]?.trim() ?? ''; + const e = parts[1]?.trim() ?? ''; + const start = s && dayjs(s).isValid() ? dayjs(s) : null; + const end = e && dayjs(e).isValid() ? dayjs(e) : null; + // ONLY use the fast path if BOTH are valid dates. + // If either is a formula (invalid dayjs), we must use the async fetchTimeRange path. + if (start && end) { + setTriggerDates([start, end]); + return; + } + } + + // Async path: resolve formula strings (e.g. "30 days ago : now") + fetchTimeRange(value).then(({ value: resolved, error }) => { + if (!error && resolved) { + const matches = [...resolved.matchAll(RESOLVED_DATE_RE)]; + if (matches.length >= 2) { + setTriggerDates([ + dayjs(matches[0][1]).isValid() ? dayjs(matches[0][1]) : null, + dayjs(matches[1][1]).isValid() ? dayjs(matches[1][1]) : null, + ]); + } + } + }); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [filterState.value]); + useEffect(() => { + if (show && activeTab === 'basic') { + // Small delay allows the popover animation to finish before mounting + const timer = setTimeout(() => setCalendarKey(k => k + 1), 80); + return () => clearTimeout(timer); + } + return undefined; + }, [show, activeTab]); + + /* ---- Resolve actual time range preview ------------------------- */ + useEffect(() => { + let isCurrent = true; + if (!timeRangeValue || timeRangeValue === NO_TIME_RANGE) { + setEvalResponse(''); + setValidTimeRange(true); + return undefined; + } + fetchTimeRange(timeRangeValue).then(({ value: resolved, error }) => { + if (!isCurrent) return; + if (error) { + setEvalResponse(error || ''); + setValidTimeRange(false); + } else { + setEvalResponse(resolved || ''); + setValidTimeRange(true); + } + }); + return () => { + isCurrent = false; + }; + }, [timeRangeValue]); + + /* ---- Emit filter ---------------------------------------------- */ + const emitFilter = useCallback( + async (rangeStr: string) => { + const isSet = rangeStr && rangeStr !== NO_TIME_RANGE; + if (!isSet) { + setDataMask({ + extraFormData: { filters: [] }, + filterState: { value: null, label: '' }, + }); + return; + } + + const extra: JsonObject = {}; + + if (!col) { + extra.time_range = rangeStr; + } + + try { + const { value: resolved, error } = await fetchTimeRange(rangeStr); + if (!error && resolved) { + const matches = [...resolved.matchAll(RESOLVED_DATE_RE)]; + if (matches.length >= 2) { + const [[, start], [, end]] = matches; + if (col) { + extra.filters = [ + { col, op: '>=', val: start }, + { col, op: '<=', val: end }, + ]; + } + } + } else if (!col) { + extra.time_range = rangeStr; + } + } catch { + if (!col) { + extra.time_range = rangeStr; + } + } + + setDataMask({ + extraFormData: extra, + filterState: { value: rangeStr, label: rangeStr }, + }); + }, + [col, setDataMask], + ); + + /* ---- Popover lifecycle ----------------------------------------- */ + function onOpen() { + const current = (filterState.value as string) || NO_TIME_RANGE; + setTimeRangeValue(current); + + // Compute defaultPickerValue once upon opening based on current value + let endVal: dayjs.Dayjs | null = null; + + if (current && current !== NO_TIME_RANGE && current.includes(SEPARATOR)) { + const parts = current.split(SEPARATOR); + const e = parts[1]?.trim() || ''; + if (e && dayjs(e).isValid()) { + endVal = dayjs(e); + } + } + + const end = endVal || dayjs(); + if (end.isValid()) { + setDefaultPickerValue([end.subtract(1, 'month'), end]); + } else { + setDefaultPickerValue(undefined); + } + + // Always open on Basic tab so the inline calendar is shown immediately + setActiveTab('basic'); + setShow(true); + setFilterActive(true); + } + + function onClose() { + setShow(false); + setFilterActive(false); + unsetFocusedFilter(); + unsetHoveredFilter(); + } + + async function onApply() { + await emitFilter(timeRangeValue); + onClose(); + } + + function onTabChange(key: TabKey) { + setActiveTab(key); + if (key !== 'basic') { + setTimeRangeValue(NO_TIME_RANGE); + } + } + + /* ---- Apply on mount if existing value -------------------------- */ + useEffect(() => { + if (filterState.value) { + emitFilter(filterState.value as string); + } + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + + /* ---- Tab content ---------------------------------------------- */ + const frameProps = { + value: timeRangeValue, + onChange: setTimeRangeValue, + isOverflowingFilterBar, + }; + + // ---- Helpers ---- + const isDynamic = (val: string) => { + if (!val) return false; + const lower = val.toLowerCase(); + if ( + lower === 'now' || + lower === 'today' || + lower.includes('ago') || + lower.includes('last') + ) + return true; + return !/^\d{4}-\d{2}-\d{2}/.test(val); + }; + + const isRangeDynamic = isDynamic(since) || isDynamic(until); + + const renderBasicTab = () => ( + <AntdThemeProvider locale={datePickerLocale ?? undefined}> + {/* Inline dual-month calendar */} + <InlineCalendarContainer ref={calendarContainerRef}> + <RangePicker + key={calendarKey} + open + value={calValue} + defaultPickerValue={defaultPickerValue} + onCalendarChange={( + dates: [dayjs.Dayjs | null, dayjs.Dayjs | null] | null, + ) => { + if (dates && (dates[0] || dates[1])) { + const start = dates[0] ? dates[0].format('YYYY-MM-DD') : ''; + const end = dates[1] ? dates[1].format('YYYY-MM-DD') : ''; + setTimeRangeValue(`${start}${SEPARATOR}${end}`); + if (dates[0]) { + setDefaultPickerValue([dates[0], dates[0].add(1, 'month')]); + } else if (dates[1]) { + setDefaultPickerValue([ + dates[1].subtract(1, 'month'), + dates[1], + ]); + } + } else { + setTimeRangeValue(NO_TIME_RANGE); + setDefaultPickerValue(undefined); + } + }} + onChange={( + dates: [dayjs.Dayjs | null, dayjs.Dayjs | null] | null, + ) => { + if (dates && dates[0] && dates[1]) { + setTimeRangeValue( + `${dates[0].format('YYYY-MM-DD')}${SEPARATOR}${dates[1].format('YYYY-MM-DD')}`, + ); + setDefaultPickerValue([dates[0], dates[1]]); + } else { + setTimeRangeValue(NO_TIME_RANGE); + setDefaultPickerValue(undefined); + } + }} + allowClear={false} + getPopupContainer={() => + calendarContainerRef.current ?? document.body + } + /> + </InlineCalendarContainer> + + {/* Start / End text inputs with formula hints */} + <div style={{ display: 'flex', gap: 16, boxSizing: 'border-box' }}> + <div style={{ flex: '1 1 0', minWidth: 0 }}> + <div className="control-label"> + {t('Start (inclusive)')}{' '} + <InfoTooltip + tooltip={t('Start date included in time range')} + placement="right" + /> + </div> + <InputWrapper> + <input + className="ant-input" + value={since} + onChange={e => + setTimeRangeValue(`${e.target.value}${SEPARATOR}${until}`) + } + placeholder={t('Select in calendar or type')} + style={{ + width: '100%', + boxSizing: 'border-box', + paddingRight: 30, + }} + /> + {since && ( + <button + type="button" + className="clear-icon" + onClick={() => setTimeRangeValue(`${SEPARATOR}${until}`)} + style={{ + background: 'none', + border: 'none', + padding: 0, + }} + > + ✕ + </button> + )} + </InputWrapper> + <div style={{ marginTop: 2, fontSize: 11, opacity: 0.6 }}> + {t('Suggested:')}{' '} + <button + type="button" + style={{ + background: 'none', + border: 'none', + padding: 0, + color: theme.colorPrimary, + cursor: 'pointer', + font: 'inherit', + textDecoration: 'none', + display: 'inline', + }} + onClick={() => + setTimeRangeValue(`7 days ago${SEPARATOR}${until}`) + } + > + {t('7 days ago')} + </button> + {', '} + <button + type="button" + style={{ + background: 'none', + border: 'none', + padding: 0, + color: theme.colorPrimary, + cursor: 'pointer', + font: 'inherit', + textDecoration: 'none', + display: 'inline', + }} + onClick={() => + setTimeRangeValue(`30 days ago${SEPARATOR}${until}`) + } + > + {t('30 days ago')} + </button> + {', '} + <button + type="button" + style={{ + background: 'none', + border: 'none', + padding: 0, + color: theme.colorPrimary, + cursor: 'pointer', + font: 'inherit', + textDecoration: 'none', + display: 'inline', + }} + onClick={() => + setTimeRangeValue(`45 days ago${SEPARATOR}${until}`) + } + > + {t('45 days ago')} + </button> + {', '} + <button + type="button" + style={{ + background: 'none', + border: 'none', + padding: 0, + color: theme.colorPrimary, + cursor: 'pointer', + font: 'inherit', + textDecoration: 'none', + display: 'inline', + }} + onClick={() => + setTimeRangeValue(`90 days ago${SEPARATOR}${until}`) + } + > + {t('90 days ago')} + </button> + </div> Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing aria-label on suggested buttons</b></div> <div id="fix"> The 8 suggested-range buttons lack `aria-label` attributes, so screen readers announce only the generic role ('button') without the visible label text. Adding `aria-label` ensures accessible names for all users. </div> </div> <div id="suggestion"> <div id="issue"><b>Inline style duplication in buttons</b></div> <div id="fix"> The 4 Start-time suggested buttons (lines 724-793) all use identical inline style blocks (10 lines each, 40 lines total). Extracting to a shared constant reduces duplication and simplifies future style updates. </div> </div> <small><i>Code Review Run #e41105</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]
