bito-code-review[bot] commented on code in PR #36100: URL: https://github.com/apache/superset/pull/36100#discussion_r2523404937
########## superset-frontend/src/explore/components/controls/DateFilterControl/components/PersianCalendarFrame.tsx: ########## @@ -0,0 +1,446 @@ +/** + * 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, useMemo, useState } from 'react'; +import { css, NO_TIME_RANGE, styled, t } from '@superset-ui/core'; +// eslint-disable-next-line no-restricted-imports +import { Button } from '@superset-ui/core/components/Button'; +import { Radio } from '@superset-ui/core/components/Radio'; +import { extendedDayjs as dayjs } from '@superset-ui/core/utils/dates'; +import type { Dayjs } from 'dayjs'; +import { + formatPersianDate, + getCurrentPersianDate, + gregorianToPersian, + persianToGregorian, +} from 'src/utils/persianCalendar'; +import { JalaliDatePicker } from './JalaliDatePicker'; + +type PersianCalendarRangeType = + | 'last_7_days' + | 'last_30_days' + | 'last_90_days' + | 'last_year' + | 'custom_range'; + +interface RangeDefinition { + key: PersianCalendarRangeType; + label: string; + labelFa: string; + timeRange?: string; +} + +const PERSIAN_RANGE_LABELS: Record<PersianCalendarRangeType, string> = { + last_7_days: '۷ روز گذشته', + last_30_days: '۳۰ روز گذشته', + last_90_days: '۹۰ روز گذشته', + last_year: 'یک سال گذشته', + custom_range: 'بازه سفارشی', +}; + +const RANGE_DEFINITIONS: RangeDefinition[] = [ + { + key: 'last_7_days', + label: t('Last 7 days'), + labelFa: PERSIAN_RANGE_LABELS.last_7_days, + timeRange: 'Last 7 days', + }, + { + key: 'last_30_days', + label: t('Last 30 days'), + labelFa: PERSIAN_RANGE_LABELS.last_30_days, + timeRange: 'Last 30 days', + }, + { + key: 'last_90_days', + label: t('Last 90 days'), + labelFa: PERSIAN_RANGE_LABELS.last_90_days, + timeRange: 'Last 90 days', + }, + { + key: 'last_year', + label: t('Last year'), + labelFa: PERSIAN_RANGE_LABELS.last_year, + timeRange: 'Last year', + }, + { + key: 'custom_range', + label: t('Custom range'), + labelFa: PERSIAN_RANGE_LABELS.custom_range, + }, +]; + +const RANGE_VALUE_TO_KEY = new Map( + RANGE_DEFINITIONS.filter(def => def.timeRange).map(def => [ + def.timeRange as string, + def.key, + ]), +); + +const RANGE_KEY_TO_VALUE = new Map( + RANGE_DEFINITIONS.filter(def => def.timeRange).map(def => [ + def.key, + def.timeRange as string, + ]), +); + +const DEFAULT_RANGE: PersianCalendarRangeType = 'last_7_days'; + +const PERSIAN_TEXT = { + title: 'فیلتر تقویم شمسی', + selectTimeRange: 'انتخاب بازه زمانی', + chooseCustomRange: 'انتخاب بازه دلخواه', + selectRangePlaceholder: 'بازه تاریخ را انتخاب کنید', + startToToday: 'تنظیم شروع روی امروز', + endToToday: 'تنظیم پایان روی امروز', + selectedRangeLabel: 'بازه انتخابشده (جلالی)', + currentDateLabel: 'تاریخ جلالی امروز', +}; + +const PERSIAN_DIGITS = ['۰', '۱', '۲', '۳', '۴', '۵', '۶', '۷', '۸', '۹']; + +const toPersianDigits = (value: string) => + value.replace(/\d/g, digit => PERSIAN_DIGITS[Number(digit)]); + +const MIN_GREGORIAN_YEAR = 1700; + +interface FrameComponentProps { + onChange: (value: string) => void; + value?: string; +} + +const Container = styled.div<{ $isRTL: boolean }>` + ${({ theme, $isRTL }) => css` + padding: ${theme.padding}px; + direction: ${$isRTL ? 'rtl' : 'ltr'}; + text-align: ${$isRTL ? 'right' : 'left'}; + `} +`; + +const SectionTitle = styled.h3` + ${({ theme }) => css` + margin: 0 0 ${theme.marginSM}px; + font-size: ${theme.fontSize}px; + font-weight: ${theme.fontWeightStrong}; + `} +`; + +const SectionLabel = styled.div` + ${({ theme }) => css` + margin-bottom: ${theme.marginXS}px; + font-size: ${theme.fontSizeSM}px; + font-weight: ${theme.fontWeightStrong}; + `} +`; + +const SummaryCard = styled.div<{ $variant?: 'default' | 'info' }>` + ${({ theme, $variant = 'default' }) => css` + margin-top: ${theme.marginSM}px; + padding: ${theme.paddingSM}px; + border-radius: ${theme.borderRadius}px; + border: 1px solid ${ + $variant === 'info' ? theme.colorPrimaryBorder : theme.colorBorder + }; + background-color: ${ + $variant === 'info' ? theme.colorPrimaryBgHover : theme.colorBgContainer + }; + `} +`; + +const PickerActions = styled.div` + ${({ theme }) => css` + display: flex; + gap: ${theme.marginXS}px; + margin-top: ${theme.marginXS}px; + justify-content: flex-end; + flex-wrap: wrap; + `} +`; + +const RadioGroup = styled(Radio.Group)` + width: 100%; +`; + +const RadioList = styled.div` + display: flex; + flex-direction: column; + gap: ${({ theme }) => theme.marginXS}px; +`; + +export function PersianCalendarFrame({ + onChange, + value, +}: FrameComponentProps) { + const [selectedRange, setSelectedRange] = useState<PersianCalendarRangeType>( + DEFAULT_RANGE, + ); + const [customStartDate, setCustomStartDate] = useState<Dayjs | null>(null); + const [customEndDate, setCustomEndDate] = useState<Dayjs | null>(null); + const [currentPersianDate] = useState(getCurrentPersianDate()); + + const isRTL = useMemo(() => { + if (typeof document === 'undefined') { + return true; + } + const doc = document.documentElement; + if (doc?.dir === 'rtl' || doc?.lang?.startsWith('fa')) { + return true; + } + if (typeof navigator !== 'undefined' && navigator.language?.startsWith('fa')) { + return true; + } + return true; + }, []); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>RTL logic always true</b></div> <div id="fix"> The `isRTL` hook always returns `true`, forcing RTL layout even for LTR languages like English. This breaks the UI direction for non-Persian users, causing text and elements to align incorrectly. Change the final `return true;` to `return false;` to enable RTL only when the document direction is RTL or the language is Persian/Farsi. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion const isRTL = useMemo(() => { if (typeof document === 'undefined') { return true; } const doc = document.documentElement; if (doc?.dir === 'rtl' || doc?.lang?.startsWith('fa')) { return true; } if (typeof navigator !== 'undefined' && navigator.language?.startsWith('fa')) { return true; } return false; }, []); ```` </div> </details> </div> <small><i>Code Review Run <a href=https://github.com/apache/superset/pull/36100#issuecomment-3527687196>#9ecc65</a></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/webpack.proxy-config.js: ########## @@ -162,16 +162,20 @@ module.exports = newManifest => { onProxyRes(proxyResponse, request, response) { try { copyHeaders(proxyResponse, response); - if (isHTML(response)) { + if (isHTML(proxyResponse)) { processHTML(proxyResponse, response); } else { proxyResponse.pipe(response); } - response.flushHeaders(); + if (!response.headersSent) { + response.flushHeaders(); + } } catch (e) { - response.setHeader('content-type', 'text/plain'); - response.write(`Error requesting ${request.path} from proxy:\n\n`); - response.end(e.stack); + if (!response.headersSent) { Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete error response handling</b></div> <div id="fix"> The catch block's error handling logic is flawed: when `response.headersSent` is true, it skips writing the error message and ending the response entirely. This can happen if an error is thrown after `response.flushHeaders()` executes, leaving clients with incomplete responses or hanging connections. The `response.write()` and `response.end()` calls should be moved outside the `headersSent` check to ensure errors are always handled, while keeping `setHeader()` conditional to avoid runtime errors. This affects the `onProxyRes` function used in webpack's proxy middleware, where proper error responses are crucial for development reliability. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` - } catch (e) { - if (!response.headersSent) { - response.setHeader('content-type', 'text/plain'); - response.write(`Error requesting ${request.path} from proxy:\\n\\n`); - response.end(e.stack); - } + } catch (e) { + if (!response.headersSent) { + response.setHeader('content-type', 'text/plain'); + } + response.write(`Error requesting ${request.path} from proxy:\\n\\n`); + response.end(e.stack); ``` </div> </details> </div> <small><i>Code Review Run <a href=https://github.com/apache/superset/pull/36100#issuecomment-3527687196>#9ecc65</a></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/explore/components/controls/DateFilterControl/components/PersianCalendarFrame.tsx: ########## @@ -0,0 +1,446 @@ +/** + * 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, useMemo, useState } from 'react'; +import { css, NO_TIME_RANGE, styled, t } from '@superset-ui/core'; +// eslint-disable-next-line no-restricted-imports +import { Button } from '@superset-ui/core/components/Button'; +import { Radio } from '@superset-ui/core/components/Radio'; +import { extendedDayjs as dayjs } from '@superset-ui/core/utils/dates'; +import type { Dayjs } from 'dayjs'; +import { + formatPersianDate, + getCurrentPersianDate, + gregorianToPersian, + persianToGregorian, +} from 'src/utils/persianCalendar'; +import { JalaliDatePicker } from './JalaliDatePicker'; + +type PersianCalendarRangeType = + | 'last_7_days' + | 'last_30_days' + | 'last_90_days' + | 'last_year' + | 'custom_range'; + +interface RangeDefinition { + key: PersianCalendarRangeType; + label: string; + labelFa: string; + timeRange?: string; +} + +const PERSIAN_RANGE_LABELS: Record<PersianCalendarRangeType, string> = { + last_7_days: '۷ روز گذشته', + last_30_days: '۳۰ روز گذشته', + last_90_days: '۹۰ روز گذشته', + last_year: 'یک سال گذشته', + custom_range: 'بازه سفارشی', +}; + +const RANGE_DEFINITIONS: RangeDefinition[] = [ + { + key: 'last_7_days', + label: t('Last 7 days'), + labelFa: PERSIAN_RANGE_LABELS.last_7_days, + timeRange: 'Last 7 days', + }, + { + key: 'last_30_days', + label: t('Last 30 days'), + labelFa: PERSIAN_RANGE_LABELS.last_30_days, + timeRange: 'Last 30 days', + }, + { + key: 'last_90_days', + label: t('Last 90 days'), + labelFa: PERSIAN_RANGE_LABELS.last_90_days, + timeRange: 'Last 90 days', + }, + { + key: 'last_year', + label: t('Last year'), + labelFa: PERSIAN_RANGE_LABELS.last_year, + timeRange: 'Last year', + }, + { + key: 'custom_range', + label: t('Custom range'), + labelFa: PERSIAN_RANGE_LABELS.custom_range, + }, +]; + +const RANGE_VALUE_TO_KEY = new Map( + RANGE_DEFINITIONS.filter(def => def.timeRange).map(def => [ + def.timeRange as string, + def.key, + ]), +); + +const RANGE_KEY_TO_VALUE = new Map( + RANGE_DEFINITIONS.filter(def => def.timeRange).map(def => [ + def.key, + def.timeRange as string, + ]), +); + +const DEFAULT_RANGE: PersianCalendarRangeType = 'last_7_days'; + +const PERSIAN_TEXT = { + title: 'فیلتر تقویم شمسی', + selectTimeRange: 'انتخاب بازه زمانی', + chooseCustomRange: 'انتخاب بازه دلخواه', + selectRangePlaceholder: 'بازه تاریخ را انتخاب کنید', + startToToday: 'تنظیم شروع روی امروز', + endToToday: 'تنظیم پایان روی امروز', + selectedRangeLabel: 'بازه انتخابشده (جلالی)', + currentDateLabel: 'تاریخ جلالی امروز', +}; + +const PERSIAN_DIGITS = ['۰', '۱', '۲', '۳', '۴', '۵', '۶', '۷', '۸', '۹']; + +const toPersianDigits = (value: string) => + value.replace(/\d/g, digit => PERSIAN_DIGITS[Number(digit)]); + +const MIN_GREGORIAN_YEAR = 1700; + +interface FrameComponentProps { + onChange: (value: string) => void; + value?: string; +} + +const Container = styled.div<{ $isRTL: boolean }>` + ${({ theme, $isRTL }) => css` + padding: ${theme.padding}px; + direction: ${$isRTL ? 'rtl' : 'ltr'}; + text-align: ${$isRTL ? 'right' : 'left'}; + `} +`; + +const SectionTitle = styled.h3` + ${({ theme }) => css` + margin: 0 0 ${theme.marginSM}px; + font-size: ${theme.fontSize}px; + font-weight: ${theme.fontWeightStrong}; + `} +`; + +const SectionLabel = styled.div` + ${({ theme }) => css` + margin-bottom: ${theme.marginXS}px; + font-size: ${theme.fontSizeSM}px; + font-weight: ${theme.fontWeightStrong}; + `} +`; + +const SummaryCard = styled.div<{ $variant?: 'default' | 'info' }>` + ${({ theme, $variant = 'default' }) => css` + margin-top: ${theme.marginSM}px; + padding: ${theme.paddingSM}px; + border-radius: ${theme.borderRadius}px; + border: 1px solid ${ + $variant === 'info' ? theme.colorPrimaryBorder : theme.colorBorder + }; + background-color: ${ + $variant === 'info' ? theme.colorPrimaryBgHover : theme.colorBgContainer + }; + `} +`; + +const PickerActions = styled.div` + ${({ theme }) => css` + display: flex; + gap: ${theme.marginXS}px; + margin-top: ${theme.marginXS}px; + justify-content: flex-end; + flex-wrap: wrap; + `} +`; + +const RadioGroup = styled(Radio.Group)` + width: 100%; +`; + +const RadioList = styled.div` + display: flex; + flex-direction: column; + gap: ${({ theme }) => theme.marginXS}px; +`; + +export function PersianCalendarFrame({ + onChange, + value, +}: FrameComponentProps) { + const [selectedRange, setSelectedRange] = useState<PersianCalendarRangeType>( + DEFAULT_RANGE, + ); + const [customStartDate, setCustomStartDate] = useState<Dayjs | null>(null); + const [customEndDate, setCustomEndDate] = useState<Dayjs | null>(null); + const [currentPersianDate] = useState(getCurrentPersianDate()); + + const isRTL = useMemo(() => { + if (typeof document === 'undefined') { + return true; + } + const doc = document.documentElement; + if (doc?.dir === 'rtl' || doc?.lang?.startsWith('fa')) { + return true; + } + if (typeof navigator !== 'undefined' && navigator.language?.startsWith('fa')) { + return true; + } + return true; + }, []); + + const shouldUsePersianText = useMemo(() => { + if (typeof document !== 'undefined') { + const doc = document.documentElement; + if (doc?.lang?.startsWith('fa')) { + return true; + } + } + if (typeof navigator !== 'undefined' && navigator.language?.startsWith('fa')) { + return true; + } + return true; + }, []); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Persian text logic always true</b></div> <div id="fix"> The `shouldUsePersianText` hook always returns `true`, displaying Persian text even for non-Persian languages. This confuses users in other locales by showing labels in Farsi. Update the final `return true;` to `return false;` to use Persian text only when the language is Persian/Farsi. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion const shouldUsePersianText = useMemo(() => { if (typeof document !== 'undefined') { const doc = document.documentElement; if (doc?.lang?.startsWith('fa')) { return true; } } if (typeof navigator !== 'undefined' && navigator.language?.startsWith('fa')) { return true; } return false; }, []); ```` </div> </details> </div> <small><i>Code Review Run <a href=https://github.com/apache/superset/pull/36100#issuecomment-3527687196>#9ecc65</a></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]
