codeant-ai-for-open-source[bot] commented on code in PR #37064:
URL: https://github.com/apache/superset/pull/37064#discussion_r2683917757
##########
superset-frontend/plugins/legacy-plugin-chart-calendar/src/utils.ts:
##########
@@ -26,5 +26,11 @@ export const getFormattedUTCTime = (
) => {
const date = new Date(ts);
const offset = date.getTimezoneOffset() * 60 * 1000;
- return getTimeFormatter(timeFormat)(date.getTime() - offset);
+ return getTimeFormatter(timeFormat)(date.getTime() + offset);
Review Comment:
**Suggestion:** Logic error: the timezone offset is added instead of
subtracted in `getFormattedUTCTime`. When converting a UTC timestamp to the
equivalent local wall-clock time you must subtract the local timezone offset
(UTC - local = offset); adding the offset shifts the time in the wrong
direction and will produce incorrect displayed dates/months. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
return getTimeFormatter(timeFormat)(date.getTime() - offset);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current code adds the timezone offset. Date.prototype.getTimezoneOffset()
returns the minutes you must add to local time to get UTC, so local = UTC -
offset.
In practice that means you must subtract the offsetMs to compute the local
wall-clock
milliseconds for a given UTC instant. Adding the offset reverses the
direction and
will display the wrong local time. Switching to `date.getTime() - offset`
fixes a
real logic bug visible in the PR.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/legacy-plugin-chart-calendar/src/utils.ts
**Line:** 29:29
**Comment:**
*Logic Error: Logic error: the timezone offset is added instead of
subtracted in `getFormattedUTCTime`. When converting a UTC timestamp to the
equivalent local wall-clock time you must subtract the local timezone offset
(UTC - local = offset); adding the offset shifts the time in the wrong
direction and will produce incorrect displayed dates/months.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/legacy-plugin-chart-calendar/src/Calendar.js:
##########
@@ -105,7 +106,7 @@ function Calendar(element, props) {
const cal = new CalHeatMap();
cal.init({
- start: data.start,
+ start: convertUTCTimestampToLocal(data.start),
Review Comment:
**Suggestion:** Passing `data.start` directly to
`convertUTCTimestampToLocal` without checking it exists can throw if `data` or
`data.start` is undefined (for example when `data` is missing). Guard the call
so `convertUTCTimestampToLocal` is only invoked when `data.start` is a number.
[null pointer]
**Severity Level:** Minor ⚠️
```suggestion
start: typeof data?.start === 'number' ?
convertUTCTimestampToLocal(data.start) : undefined,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Guarding against missing data.start is a reasonable defensive change —
calling convertUTCTimestampToLocal with undefined could throw depending on its
implementation. The suggested check (typeof data?.start === 'number') prevents
a potential runtime error without changing semantics when start is present.
This is a local, low-risk improvement.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/legacy-plugin-chart-calendar/src/Calendar.js
**Line:** 109:109
**Comment:**
*Null Pointer: Passing `data.start` directly to
`convertUTCTimestampToLocal` without checking it exists can throw if `data` or
`data.start` is undefined (for example when `data` is missing). Guard the call
so `convertUTCTimestampToLocal` is only invoked when `data.start` is a number.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/legacy-plugin-chart-calendar/src/utils.ts:
##########
@@ -26,5 +26,11 @@ export const getFormattedUTCTime = (
) => {
const date = new Date(ts);
const offset = date.getTimezoneOffset() * 60 * 1000;
- return getTimeFormatter(timeFormat)(date.getTime() - offset);
+ return getTimeFormatter(timeFormat)(date.getTime() + offset);
+};
+
+export const convertUTCTimestampToLocal = (utcTimestamp: number): number => {
+ const date = new Date(utcTimestamp);
+ const offsetMs = date.getTimezoneOffset() * 60 * 1000;
+ return utcTimestamp + offsetMs;
Review Comment:
**Suggestion:** Logic error: `convertUTCTimestampToLocal` adds the timezone
offset to the UTC timestamp instead of subtracting it. To get the local
millisecond timestamp for a given UTC millisecond instant you must subtract
`date.getTimezoneOffset() * 60 * 1000`; adding will shift the instant in the
wrong direction and produce incorrect local dates. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
return utcTimestamp - offsetMs;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Same root cause as the first suggestion: the code adds offsetMs. To convert
a UTC millisecond
instant to the equivalent local millisecond wall-clock instant you must
subtract the
timezone offset (local = UTC - offset). The current addition shifts times in
the wrong
direction and is a genuine bug.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/legacy-plugin-chart-calendar/src/utils.ts
**Line:** 35:35
**Comment:**
*Logic Error: Logic error: `convertUTCTimestampToLocal` adds the
timezone offset to the UTC timestamp instead of subtracting it. To get the
local millisecond timestamp for a given UTC millisecond instant you must
subtract `date.getTimezoneOffset() * 60 * 1000`; adding will shift the instant
in the wrong direction and produce incorrect local dates.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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]
