korbit-ai[bot] commented on code in PR #31310: URL: https://github.com/apache/superset/pull/31310#discussion_r1890580860
########## superset-frontend/src/utils/dates.ts: ########## @@ -0,0 +1,69 @@ +/** + * 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 dayjs, { Dayjs } from 'dayjs'; +import utc from 'dayjs/plugin/utc'; +import timezone from 'dayjs/plugin/timezone'; +import calendar from 'dayjs/plugin/calendar'; +import relativeTime from 'dayjs/plugin/relativeTime'; +import customParseFormat from 'dayjs/plugin/customParseFormat'; +import duration from 'dayjs/plugin/duration'; +import updateLocale from 'dayjs/plugin/updateLocale'; + +dayjs.extend(utc); +dayjs.extend(timezone); +dayjs.extend(calendar); +dayjs.extend(relativeTime); +dayjs.extend(customParseFormat); +dayjs.extend(duration); +dayjs.extend(updateLocale); + +export const extendedDayjs = dayjs; + +export const fDuration = function ( + t1: number, + t2: number, + format = 'HH:mm:ss.SSS', +): string { + const diffSec = t2 - t1; + const duration = dayjs(new Date(diffSec)); + return duration.utc().format(format); +}; Review Comment: ### Incorrect Duration Calculation Method <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The fDuration function incorrectly calculates duration by creating a date from the difference in milliseconds instead of using dayjs.duration() ###### Why this matters This will produce incorrect duration formatting as it treats the millisecond difference as an epoch timestamp rather than a duration, leading to unexpected results when formatting the time difference. ###### Suggested change export const fDuration = function ( t1: number, t2: number, format = 'HH:mm:ss.SSS', ): string { return dayjs.duration(t2 - t1).format(format); }; </details> ###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews. <!--- korbi internal id:cd78f9ef-49b0-46cc-ad57-815d2a6f0100 --> ########## superset-frontend/src/preamble.ts: ########## @@ -44,7 +44,7 @@ const bootstrapData = getBootstrapData(); // Configure translation if (typeof window !== 'undefined') { configure({ languagePack: bootstrapData.common.language_pack }); - moment.locale(bootstrapData.common.locale); + dayjs.locale(bootstrapData.common.locale); Review Comment: ### Missing Day.js Locale Plugin <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Day.js locale support requires loading the appropriate locale plugin before setting the locale. ###### Why this matters Without loading the locale plugin first, the locale setting will have no effect, causing date formatting to fallback to default English locale regardless of user preferences. ###### Suggested change Import and load the Day.js locale plugin before setting the locale: ```typescript import dayjs from 'dayjs'; import LocalizedFormat from 'dayjs/plugin/localizedFormat'; dayjs.extend(LocalizedFormat); ``` </details> ###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews. <!--- korbi internal id:cf584d6f-ca4c-446c-8df7-7d8f75116fbc --> ########## superset-frontend/src/utils/dates.ts: ########## @@ -0,0 +1,69 @@ +/** + * 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 dayjs, { Dayjs } from 'dayjs'; +import utc from 'dayjs/plugin/utc'; +import timezone from 'dayjs/plugin/timezone'; +import calendar from 'dayjs/plugin/calendar'; +import relativeTime from 'dayjs/plugin/relativeTime'; +import customParseFormat from 'dayjs/plugin/customParseFormat'; +import duration from 'dayjs/plugin/duration'; +import updateLocale from 'dayjs/plugin/updateLocale'; + +dayjs.extend(utc); +dayjs.extend(timezone); +dayjs.extend(calendar); +dayjs.extend(relativeTime); +dayjs.extend(customParseFormat); +dayjs.extend(duration); +dayjs.extend(updateLocale); + +export const extendedDayjs = dayjs; + +export const fDuration = function ( + t1: number, + t2: number, + format = 'HH:mm:ss.SSS', +): string { + const diffSec = t2 - t1; + const duration = dayjs(new Date(diffSec)); + return duration.utc().format(format); +}; + +export const now = function (): number { + // seconds from EPOCH as a float + return dayjs().utc().valueOf(); +}; Review Comment: ### Incorrect Time Unit Documentation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The comment is incorrect as valueOf() returns milliseconds since epoch, not seconds ###### Why this matters This misleading comment could cause integration issues if other developers rely on the comment's description of the return value being in seconds. ###### Suggested change export const now = function (): number { // milliseconds from EPOCH as a number return dayjs().utc().valueOf(); }; </details> ###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews. <!--- korbi internal id:05617758-06ae-44ac-8c78-806e0b4f2506 --> ########## superset-frontend/src/components/TimezoneSelector/index.tsx: ########## @@ -82,43 +70,40 @@ export default function TimezoneSelector({ const getTimezoneName = (name: string) => { const offsets = getOffsetKey(name); return ( - (currentDate.tz(name).isDST() + (isDST(currentDate.tz(name), name) Review Comment: ### Redundant Timezone Conversion <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Double timezone conversion might occur as the date is already converted to the timezone before being passed to isDST, which might also perform a timezone conversion. ###### Why this matters Unnecessary timezone conversions can lead to edge case bugs around DST boundaries and increase the chance of timezone-related calculation errors. ###### Suggested change Pass the unconverted currentDate to isDST and let it handle the timezone conversion: isDST(currentDate, name) </details> ###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews. <!--- korbi internal id:c6d0965a-b8f4-4571-8987-059d1f0bf8d5 --> -- 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]
