Copilot commented on code in PR #16489: URL: https://github.com/apache/pinot/pull/16489#discussion_r2250126164
########## pinot-controller/src/main/resources/app/components/TimezoneSelector.tsx: ########## @@ -0,0 +1,187 @@ +/** + * 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 React, { useState, useEffect } from 'react'; +import { + Select, + MenuItem, + FormControl, + InputLabel, + makeStyles, + Typography, + Box, + TextField, + ListItemText, + Divider, +} from '@material-ui/core'; +import { AccessTime as AccessTimeIcon } from '@material-ui/icons'; +import { useTimezone } from '../contexts/TimezoneContext'; +import { COMMON_TIMEZONES, getAllTimezones, getTimezoneDisplayName } from '../utils/TimezoneUtils'; + +const useStyles = makeStyles((theme) => ({ + formControl: { + minWidth: 200, + margin: theme.spacing(1), + }, + select: { + '& .MuiSelect-select': { + display: 'flex', + alignItems: 'center', + }, + }, + menuItem: { + display: 'flex', + justifyContent: 'space-between', + alignItems: 'center', + }, + searchField: { + margin: theme.spacing(1), + width: '100%', + }, + divider: { + margin: theme.spacing(1, 0), + }, + timezoneLabel: { + display: 'flex', + alignItems: 'center', + gap: theme.spacing(1), + }, +})); + +interface TimezoneSelectorProps { + variant?: 'outlined' | 'filled' | 'standard'; + size?: 'small' | 'medium'; + showIcon?: boolean; +} + +const TimezoneSelector: React.FC<TimezoneSelectorProps> = ({ + variant = 'outlined', + size = 'small', + showIcon = true, +}) => { + const classes = useStyles(); + const { currentTimezone, setTimezone } = useTimezone(); + const [searchTerm, setSearchTerm] = useState(''); + const [allTimezones, setAllTimezones] = useState<Array<{ value: string; label: string }>>([]); + const [filteredTimezones, setFilteredTimezones] = useState<Array<{ value: string; label: string }>>([]); + + useEffect(() => { + // Load all timezones + const timezones = getAllTimezones(); + setAllTimezones(timezones); + setFilteredTimezones(timezones); + }, []); + + useEffect(() => { + // Filter timezones based on search term + if (!searchTerm.trim()) { + setFilteredTimezones(allTimezones); + } else { + const filtered = allTimezones.filter(tz => + tz.value.toLowerCase().includes(searchTerm.toLowerCase()) || + tz.label.toLowerCase().includes(searchTerm.toLowerCase()) + ); + setFilteredTimezones(filtered); + } + }, [searchTerm, allTimezones]); + + const handleTimezoneChange = (event: React.ChangeEvent<{ value: unknown }>) => { + const newTimezone = event.target.value as string; + setTimezone(newTimezone); + }; + + const renderMenuItem = (timezone: { value: string; label: string }) => ( + <MenuItem key={timezone.value} value={timezone.value} className={classes.menuItem}> + <Box className={classes.timezoneLabel}> + {showIcon && <AccessTimeIcon fontSize="small" />} + <Typography variant="body2">{timezone.value}</Typography> + </Box> + <Typography variant="caption" color="textSecondary"> + {timezone.label.split('(')[1]?.replace(')', '')} + </Typography> + </MenuItem> + ); + + return ( + <FormControl variant={variant} size={size} className={classes.formControl}> + <InputLabel>Timezone</InputLabel> + <Select + value={currentTimezone} + onChange={handleTimezoneChange} + className={classes.select} + MenuProps={{ + PaperProps: { + style: { + maxHeight: 400, + }, + }, + keepMounted: true, + disablePortal: true, + }} + > + {/* Common timezones section */} + <MenuItem disabled> + <Typography variant="subtitle2" color="textSecondary"> + Common Timezones + </Typography> + </MenuItem> + {COMMON_TIMEZONES.map(renderMenuItem)} + + <Divider className={classes.divider} /> + + {/* Search field */} + <Box p={1} onClick={(e) => e.stopPropagation()}> + <TextField + placeholder="Search timezones..." + value={searchTerm} + onChange={(e) => setSearchTerm(e.target.value)} + onClick={(e) => e.stopPropagation()} + onMouseDown={(e) => e.stopPropagation()} + onKeyDown={(e) => e.stopPropagation()} + onKeyUp={(e) => e.stopPropagation()} + size="small" + fullWidth + variant="outlined" + className={classes.searchField} + /> + </Box> + + <Divider className={classes.divider} /> + + {/* All timezones section */} + <MenuItem disabled> + <Typography variant="subtitle2" color="textSecondary"> + All Timezones + </Typography> + </MenuItem> + {filteredTimezones.slice(0, 100).map(renderMenuItem)} + + {filteredTimezones.length > 100 && ( + <MenuItem disabled> + <Typography variant="caption" color="textSecondary"> + Showing first 100 results. Use search to find more. Review Comment: The magic number 100 should be defined as a named constant to improve maintainability and make it easier to adjust this limit in the future. ```suggestion {filteredTimezones.slice(0, MAX_TIMEZONE_RESULTS).map(renderMenuItem)} {filteredTimezones.length > MAX_TIMEZONE_RESULTS && ( <MenuItem disabled> <Typography variant="caption" color="textSecondary"> Showing first {MAX_TIMEZONE_RESULTS} results. Use search to find more. ``` ########## pinot-controller/src/main/resources/app/utils/TimezoneUtils.ts: ########## @@ -0,0 +1,92 @@ +/** + * 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 moment from 'moment'; +import 'moment-timezone'; + +// Default timezone +export const DEFAULT_TIMEZONE = Intl.DateTimeFormat().resolvedOptions().timeZone || 'GMT'; Review Comment: The fallback to 'GMT' may not be consistent with the app_state default. Consider using a single source of truth for the default timezone value to avoid inconsistencies. ```suggestion // Default timezone fallback (single source of truth) export const DEFAULT_TIMEZONE_FALLBACK = 'GMT'; export const DEFAULT_TIMEZONE = Intl.DateTimeFormat().resolvedOptions().timeZone || DEFAULT_TIMEZONE_FALLBACK; ``` ########## pinot-controller/src/main/resources/app/pages/SegmentDetails.tsx: ########## @@ -98,11 +100,14 @@ type Props = { type Summary = { segmentName: string; totalDocs: string | number; - createTime: unknown; + createTime: number; Review Comment: Changing createTime type from 'unknown' to 'number' is a breaking change to the Summary interface. This could affect other code that depends on this interface. Consider using a union type like 'number | unknown' for backward compatibility. ```suggestion createTime: number | unknown; ``` -- 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]
