Copilot commented on code in PR #16489: URL: https://github.com/apache/pinot/pull/16489#discussion_r2250135067
########## pinot-controller/src/main/resources/app/utils/TimezoneUtils.ts: ########## @@ -0,0 +1,94 @@ +/** + * 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 fallback (single source of truth) +export const DEFAULT_TIMEZONE_FALLBACK = 'GMT'; +// Default timezone +export const DEFAULT_TIMEZONE = Intl.DateTimeFormat().resolvedOptions().timeZone || DEFAULT_TIMEZONE_FALLBACK; + +// Common timezones for easy selection +export const COMMON_TIMEZONES = [ + { value: 'GMT', label: 'GMT (UTC+0)' }, + { value: 'UTC', label: 'UTC (UTC+0)' }, + { value: 'America/New_York', label: 'Eastern Time (UTC-5/-4)' }, + { value: 'America/Chicago', label: 'Central Time (UTC-6/-5)' }, + { value: 'America/Denver', label: 'Mountain Time (UTC-7/-6)' }, + { value: 'America/Los_Angeles', label: 'Pacific Time (UTC-8/-7)' }, + { value: 'Europe/London', label: 'London (UTC+0/+1)' }, + { value: 'Europe/Paris', label: 'Paris (UTC+1/+2)' }, + { value: 'Europe/Berlin', label: 'Berlin (UTC+1/+2)' }, + { value: 'Asia/Tokyo', label: 'Tokyo (UTC+9)' }, + { value: 'Asia/Shanghai', label: 'Shanghai (UTC+8)' }, + { value: 'Asia/Kolkata', label: 'Kolkata (UTC+5:30)' }, + { value: 'Australia/Sydney', label: 'Sydney (UTC+10/+11)' }, +]; + +// Get all available timezones +export const getAllTimezones = () => { + return moment.tz.names().map(tz => ({ + value: tz, + label: `${tz} (${moment.tz(tz).format('Z')})` + })); +}; + +// Get current timezone from localStorage or default +export const getCurrentTimezone = (): string => { + return localStorage.getItem('pinot_ui:timezone') || DEFAULT_TIMEZONE; +}; + +// Set timezone in localStorage +export const setCurrentTimezone = (timezone: string): void => { + localStorage.setItem('pinot_ui:timezone', timezone); +}; + +// Format time in the current timezone +export const formatTimeInTimezone = (time: number | string | Date, format?: string, timezone?: string): string => { + const currentTimezone = timezone || getCurrentTimezone(); + const momentTime = moment(time); + + if (momentTime.isValid()) { + return momentTime.tz(currentTimezone).format(format || 'MMMM Do YYYY, HH:mm:ss'); + } + + return 'Invalid time'; +}; + +// Convert time to a specific timezone +export const convertTimeToTimezone = (time: number | string | Date, targetTimezone: string): moment.Moment => { + return moment(time).tz(targetTimezone); +}; + +// Get timezone offset string +export const getTimezoneOffset = (timezone: string): string => { + return moment.tz(timezone).format('Z'); +}; + +// Get timezone display name +export const getTimezoneDisplayName = (timezone: string): string => { + const offset = getTimezoneOffset(timezone); + return `${timezone} (${offset})`; +}; + +// Check if timezone is valid Review Comment: The isValidTimezone function is not used anywhere in the codebase but is exported. Consider adding JSDoc documentation to explain its purpose and when it should be used, or remove it if it's not needed. ```suggestion /** * Checks if the provided timezone string is a valid IANA timezone. * * Use this function to validate user input or external data before passing * the timezone to other timezone-related utilities in this module. * * @param timezone - The timezone string to validate (e.g., "America/New_York"). * @returns {boolean} True if the timezone is valid, false otherwise. */ ``` ########## 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; // Raw timestamp for timezone-aware formatting Review Comment: The Summary type definition changes createTime from `unknown` to `number`, which is a breaking change. This could cause runtime errors if the segment metadata doesn't contain a valid numeric timestamp. Consider adding validation or keeping it as `unknown` with proper type checking. ```suggestion createTime: unknown; // Raw timestamp for timezone-aware formatting; validate before use ``` ########## pinot-controller/src/main/resources/app/components/TimezoneSelector.tsx: ########## @@ -0,0 +1,190 @@ +/** + * 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'; + +// Maximum number of timezone results to display +const MAX_TIMEZONE_RESULTS = 100; Review Comment: [nitpick] The magic number 100 for MAX_TIMEZONE_RESULTS should be documented or made configurable. Consider adding a comment explaining why this limit was chosen or moving it to a configuration file. ```suggestion // Default maximum number of timezone results to display in the dropdown. // This limit is set to prevent performance issues and overwhelming the user with too many options. // Adjust as needed based on UI requirements or make configurable via props. ``` ########## pinot-controller/src/main/resources/app/pages/SegmentDetails.tsx: ########## @@ -175,9 +182,9 @@ const SegmentDetails = ({ match }: RouteComponentProps<Props>) => { setSegmentSummary({ segmentName, totalDocs: segmentMetaDataJson['segment.total.docs'] || 0, - createTime: moment(+segmentMetaDataJson['segment.creation.time']).format( - 'MMMM Do YYYY, h:mm:ss' - ), + createTime: +segmentMetaDataJson['segment.creation.time'], + startTime: segmentMetaDataJson['segment.start.time'] ? +segmentMetaDataJson['segment.start.time'] : undefined, + endTime: segmentMetaDataJson['segment.end.time'] ? +segmentMetaDataJson['segment.end.time'] : undefined, Review Comment: Using unary plus operator on segmentMetaDataJson['segment.creation.time'] without validation could result in NaN if the value is not a valid number. This should include proper validation or fallback handling. -- 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]
