feat(filters): add Persian calendar date control #36111
Conversation
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
I've completed my review and didn't find any issues.
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/src/preamble.ts | ✅ |
| superset-frontend/src/utils/persianCalendar.ts | ✅ |
| superset-frontend/src/explore/components/controls/DateFilterControl/components/JalaliDatePicker.tsx | ✅ |
| superset-frontend/src/explore/components/controls/DateFilterControl/components/PersianCalendarFrame.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #a88474
Actionable Suggestions - 1
-
superset-frontend/src/utils/persianCalendar.ts - 1
- Wrong Persian weekday calculation · Line 63-64
Review Details
-
Files reviewed - 5 · Commit Range:
3d0eaca..3d0eaca- superset-frontend/spec/explore/components/controls/DateFilterControl/PersianCalendarFrame.test.tsx
- superset-frontend/src/explore/components/controls/DateFilterControl/components/JalaliDatePicker.tsx
- superset-frontend/src/explore/components/controls/DateFilterControl/components/PersianCalendarFrame.tsx
- superset-frontend/src/preamble.ts
- superset-frontend/src/utils/persianCalendar.ts
-
Files skipped - 2
- superset-frontend/package-lock.json - Reason: Filter setting
- superset-frontend/package.json - Reason: Filter setting
-
Tools
- Eslint (Linter) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
AI Code Review powered by
| const getWeekdayName = (year: number, month: number, day: number) => | ||
| PERSIAN_WEEKDAYS[new Date(year, month - 1, day).getDay()]; |
There was a problem hiding this comment.
The weekday calculation in getWeekdayName incorrectly maps Gregorian weekdays to Persian weekdays because the PERSIAN_WEEKDAYS array starts with Saturday (index 0) while JavaScript's getDay() starts with Sunday (index 0). This causes incorrect weekday names to be returned, affecting any code that relies on gregorianToPersian for accurate date information. The fix adjusts the index using (getDay() + 1) % 7 to align the week start days properly.
Code suggestion
Check the AI-generated fix before applying
| const getWeekdayName = (year: number, month: number, day: number) => | |
| PERSIAN_WEEKDAYS[new Date(year, month - 1, day).getDay()]; | |
| const getWeekdayName = (year: number, month: number, day: number) => | |
| PERSIAN_WEEKDAYS[(new Date(year, month - 1, day).getDay() + 1) % 7]; |
Code Review Run #a88474
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Pull Request Overview
This PR adds Persian (Jalali) calendar support to Superset's date filter controls, enabling users in Persian-speaking regions to select dates using their native calendar system. The implementation wraps the existing DateFilterControl with a new PersianCalendarFrame component.
- Integrates
react-multi-date-picker,dayjs-jalali, andjalaali-jsfor accurate Jalali calendar handling - Supports both relative presets (Last 7/30/90 days, Last year) and custom date range selection
- Persists custom ranges in a stable format (YYYY-MM-DD : YYYY-MM-DD) for restoration across sessions
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
superset-frontend/src/utils/persianCalendar.ts |
New utility module providing Persian/Gregorian date conversion functions and calendar constants |
superset-frontend/src/preamble.ts |
Adds dayjs plugin and global CSS import for react-multi-date-picker |
superset-frontend/src/explore/components/controls/DateFilterControl/components/PersianCalendarFrame.tsx |
Main component implementing Persian calendar date range selection with preset and custom range modes |
superset-frontend/src/explore/components/controls/DateFilterControl/components/JalaliDatePicker.tsx |
Wrapper component around react-multi-date-picker handling date conversions between Jalali and Gregorian calendars |
superset-frontend/spec/explore/components/controls/DateFilterControl/PersianCalendarFrame.test.tsx |
Unit tests covering preset selection, custom range selection, and value persistence |
superset-frontend/package.json |
Adds runtime dependencies (dayjs-jalali, jalaali-js, react-multi-date-picker) and dev dependency (@types/jalaali-js) |
superset-frontend/package-lock.json |
Lock file updates for new dependencies and transitive dependencies |
Files not reviewed (1)
- superset-frontend/package-lock.json: Language not supported
| const convertDateObjectToDayjs = useCallback( | ||
| (date: DateObject | null): Dayjs | null => { | ||
| if (!date) { | ||
| return null; | ||
| } | ||
| const monthValue = getMonthNumber(date.month as number | Month | undefined); | ||
|
|
||
| if (jalaliReady) { | ||
| try { | ||
| const jalaliDate = dayjs( | ||
| `${date.year}/${monthValue}/${date.day}`, | ||
| 'jYYYY/jM/jD', | ||
| ); | ||
| const gregorianDate = dayjs(jalaliDate.toDate()); | ||
| if (gregorianDate.isValid()) { | ||
| return gregorianDate; | ||
| } | ||
| } catch (error) { | ||
| console.warn('Error converting Jalali to Gregorian:', error); | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| const gregorian = persianToGregorian(date.year, monthValue, date.day); | ||
| const converted = dayjs( | ||
| `${gregorian.year}-${String(gregorian.month).padStart(2, '0')}-${String( | ||
| gregorian.day, | ||
| ).padStart(2, '0')}`, | ||
| ); | ||
| return converted.isValid() ? converted : null; | ||
| } catch (error) { | ||
| console.error('Error in fallback conversion:', error); | ||
| return null; | ||
| } | ||
| }, | ||
| [jalaliReady], | ||
| ); |
There was a problem hiding this comment.
The error handling in the conversion functions only logs to console (console.warn, console.error) but doesn't provide user feedback when date conversion fails. Consider adding proper error handling that can surface errors to the user, especially for the convertDateObjectToDayjs function which silently returns null on errors.
| const getWeekdayName = (year: number, month: number, day: number) => | ||
| PERSIAN_WEEKDAYS[new Date(year, month - 1, day).getDay()]; |
There was a problem hiding this comment.
The getWeekdayName function uses Gregorian date to calculate weekday, which may produce incorrect weekday names for Persian dates. The JavaScript Date object expects Gregorian dates, but this function receives year/month/day that appear to be intended as Gregorian coordinates. However, this function is called with Gregorian dates from gregorianToPersian, so it should work correctly.
Actually, on closer inspection, this is being called correctly in line 79 with Gregorian parameters before conversion. However, the parameter names and function placement suggest it might be misunderstood. Consider adding a comment clarifying that this expects Gregorian year/month/day values.
| const getWeekdayName = (year: number, month: number, day: number) => | |
| PERSIAN_WEEKDAYS[new Date(year, month - 1, day).getDay()]; | |
| /** | |
| * Returns the Persian weekday name for a given Gregorian date. | |
| * Expects Gregorian year, month, and day as arguments. | |
| * @param gregorianYear - Gregorian year | |
| * @param gregorianMonth - Gregorian month (1-based) | |
| * @param gregorianDay - Gregorian day | |
| */ | |
| const getWeekdayName = ( | |
| gregorianYear: number, | |
| gregorianMonth: number, | |
| gregorianDay: number, | |
| ) => | |
| PERSIAN_WEEKDAYS[new Date(gregorianYear, gregorianMonth - 1, gregorianDay).getDay()]; |
superset-frontend/src/preamble.ts
Outdated
| import 'react-multi-date-picker/styles/layouts/mobile.css'; | ||
|
|
There was a problem hiding this comment.
The global CSS import in the preamble may cause styling conflicts with other parts of the application. Consider scoping this import to only the components that need it, or importing it within the JalaliDatePicker component instead of globally.
| import 'react-multi-date-picker/styles/layouts/mobile.css'; |
| <style>{` | ||
| .custom-rmdp { | ||
| position: relative; | ||
| width: 100%; | ||
| } | ||
| .custom-rmdp .rmdp-wrapper { | ||
| direction: ${isRTL ? 'rtl' : 'ltr'}; | ||
| z-index: 10000 !important; | ||
| background: white; | ||
| border: 1px solid #d9d9d9; | ||
| border-radius: 4px; | ||
| box-shadow: 0 2px 8px rgba(0, 0, 0, 0.15); | ||
| padding: 8px; | ||
| width: 100%; | ||
| max-width: 100%; | ||
| } | ||
| .custom-rmdp .rmdp-calendar { | ||
| direction: ${isRTL ? 'rtl' : 'ltr'}; | ||
| z-index: 10000 !important; | ||
| width: 100%; | ||
| max-width: 100%; | ||
| } | ||
| .custom-rmdp .rmdp-header { | ||
| padding: 8px; | ||
| border-bottom: 1px solid #f0f0f0; | ||
| margin-bottom: 8px; | ||
| } | ||
| .custom-rmdp .rmdp-header-values { | ||
| font-weight: 600; | ||
| font-size: 14px; | ||
| color: #1890ff; | ||
| } | ||
| .custom-rmdp .rmdp-arrow { | ||
| border: solid #1890ff; | ||
| border-width: 0 2px 2px 0; | ||
| padding: 3px; | ||
| cursor: pointer; | ||
| } | ||
| .custom-rmdp .rmdp-arrow:hover { | ||
| border-color: #40a9ff; | ||
| } | ||
| .custom-rmdp .rmdp-arrow-container { | ||
| cursor: pointer; | ||
| padding: 4px; | ||
| } | ||
| .custom-rmdp .rmdp-arrow-container:hover { | ||
| background-color: #f0f0f0; | ||
| border-radius: 4px; | ||
| } | ||
| .custom-rmdp .rmdp-week-day { | ||
| color: #1890ff; | ||
| font-weight: 600; | ||
| padding: 8px; | ||
| } | ||
| .custom-rmdp .rmdp-day { | ||
| cursor: pointer; | ||
| padding: 4px; | ||
| margin: 2px; | ||
| border-radius: 4px; | ||
| } | ||
| .custom-rmdp .rmdp-day:hover { | ||
| background-color: #e6f7ff !important; | ||
| } | ||
| .custom-rmdp .rmdp-day.rmdp-selected span:not(.highlight) { | ||
| background-color: #1890ff !important; | ||
| color: white !important; | ||
| border-radius: 4px; | ||
| } | ||
| .custom-rmdp .rmdp-day.rmdp-today span, | ||
| .custom-rmdp .rmdp-day-today span { | ||
| background-color: #e6f7ff !important; | ||
| border: 1px solid #1890ff !important; | ||
| font-weight: bold; | ||
| border-radius: 4px; | ||
| } | ||
| .custom-rmdp .rmdp-day.rmdp-today:hover span, | ||
| .custom-rmdp .rmdp-day-today:hover span { | ||
| background-color: #bae7ff !important; | ||
| } | ||
| .custom-rmdp .rmdp-day.rmdp-disabled { | ||
| cursor: not-allowed; | ||
| opacity: 0.5; | ||
| } | ||
| .custom-rmdp .rmdp-day.rmdp-disabled:hover { | ||
| background-color: transparent !important; | ||
| } | ||
| .jalali-date-input { | ||
| width: 100%; | ||
| cursor: pointer; | ||
| padding: 4px 11px; | ||
| border: 1px solid #d9d9d9; | ||
| border-radius: 4px; | ||
| } | ||
| .jalali-date-input:hover { | ||
| border-color: #40a9ff; | ||
| } | ||
| .jalali-date-input:focus { | ||
| border-color: #40a9ff; | ||
| outline: 0; | ||
| box-shadow: 0 0 0 2px rgba(24, 144, 255, 0.2); | ||
| } | ||
| .jalali-date-input--range { | ||
| text-align: ${isRTL ? 'right' : 'left'}; | ||
| } | ||
| .rmdp-container, | ||
| .rmdp-popup { | ||
| z-index: 10000 !important; | ||
| } | ||
| `}</style> |
There was a problem hiding this comment.
The inline <style> tag creates duplicate styles each time the component renders. This is inefficient and can cause memory leaks with repeated mounting/unmounting. Consider using styled-components, CSS modules, or moving styles to a separate stylesheet to avoid re-creating the same styles on every render.
| const detectRTL = () => { | ||
| if (typeof document === 'undefined') { | ||
| return false; | ||
| } | ||
| const doc = document.documentElement; | ||
| if (doc?.dir === 'rtl' || doc?.lang?.startsWith('fa')) { | ||
| return true; | ||
| } | ||
| if (typeof navigator !== 'undefined') { | ||
| return navigator.language?.startsWith('fa') ?? false; | ||
| } | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
The RTL detection logic is duplicated in both PersianCalendarFrame (lines 197-209) and JalaliDatePicker (lines 72-84). Consider extracting this into a shared utility function in persianCalendar.ts to follow the DRY (Don't Repeat Yourself) principle.
| useEffect(() => { | ||
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| const jalaliPlugin = require('dayjs-jalali'); | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| const jalaliDayjsModule = jalaliPlugin.default || jalaliPlugin; | ||
| dayjs.extend(jalaliDayjsModule); | ||
| setJalaliReady(true); | ||
| } catch (error) { | ||
| console.warn('Failed to load dayjs-jalali plugin:', error); | ||
| setJalaliReady(false); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
The dayjs-jalali plugin is loaded dynamically with require() but never unloaded. This means calling dayjs.extend() multiple times could have unintended side effects if the component is mounted and unmounted repeatedly. Consider checking if the plugin is already extended before calling extend(), or ensure this side effect is acceptable.
| const toPersianDigits = (value: string) => | ||
| value.replace(/\d/g, digit => PERSIAN_DIGITS[Number(digit)]); | ||
|
|
||
| const MIN_GREGORIAN_YEAR = 1700; |
There was a problem hiding this comment.
The magic number 1700 for minimum Gregorian year lacks explanation. This appears to be a threshold to distinguish between Gregorian and Jalali year values (since Jalali years are currently in the 1400s), but this assumption could break in the future. Add a constant with a descriptive name and comment explaining why this value is used, or consider a more robust way to distinguish between the two calendar systems.
| const MIN_GREGORIAN_YEAR = 1700; | |
| /** | |
| * Threshold year to distinguish between Gregorian and Jalali (Persian) years. | |
| * Jalali years are currently in the 1400s, while Gregorian years are above 1700. | |
| * This threshold is used to infer the calendar system from a year value. | |
| * If the calendars' year ranges ever overlap, this logic should be revisited. | |
| */ | |
| const GREGORIAN_YEAR_THRESHOLD_FOR_CALENDAR_TYPE = 1700; |
| const isRTL = useMemo(() => { | ||
| if (typeof document === 'undefined') { | ||
| return false; | ||
| } | ||
| 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; | ||
| }, []); | ||
|
|
||
| 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; | ||
| }, []); |
There was a problem hiding this comment.
The Persian/Farsi text detection logic is duplicated (lines 197-209 and 211-222). This code appears twice with nearly identical logic. Consider consolidating into a single computed value or extracting to a utility function.
msyavuz
left a comment
msyavuz
left a comment
There was a problem hiding this comment.
This would be a great use case for new extensions project. As a seperate component i am not sure about the maintaining this one.
2 participants
SUMMARY
PersianCalendarFramewrapper around the existing DateFilterControl so users can work with time ranges in the Jalali calendar.react-multi-date-picker,dayjs-jalaliandjalaali-jsto handle Jalali date parsing/formatting.YYYY-MM-DD : YYYY-MM-DD) so they can be restored when reopening Explore.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
After
TESTING INSTRUCTIONS
Install frontend dependencies and build:
cd superset-frontendnpm install(ornpm cidepending on project guidelines)npm run build(optional, if you want to check build status locally)Run unit tests for the new date control:
npm test -- spec/explore/components/controls/DateFilterControl/PersianCalendarFrame.test.tsxManual testing in the UI:
YYYY-MM-DD : YYYY-MM-DD.Regression checks:
ADDITIONAL INFORMATION
New dependencies (runtime + types):
dayjs-jalalijalaali-jsreact-multi-date-picker@types/jalaali-jsBackward compatibility:
Why this is useful:
توضیحات فارسی:
این PR یک کنترل تاریخ تقویم شمسی (جلالی) به فیلتر تاریخ Superset اضافه می‌کند.
از این به بعد کاربرانی که با تقویم فارسی کار می‌کنند می‌توانند:
هیچ رفتار قبلی برای کاربرانی که هنوز از تقویم میلادی استفاده می‌کنند تغییر نمی‌کند؛ این فقط یک قابلیت اضافه برای کاربران فارسی‌زبان است.