codeant-ai-for-open-source[bot] commented on code in PR #36934:
URL: https://github.com/apache/superset/pull/36934#discussion_r2670434500
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx:
##########
@@ -310,10 +312,16 @@ const Chart = props => {
return DEFAULT_HEADER_HEIGHT;
}, [headerRef]);
+ const queriedDttm = queriesResponse?.[0]?.queried_dttm ?? null;
Review Comment:
**Suggestion:** Logic error: the code reads the first element of
`queriesResponse` (`[0]`) to determine the "last queried" timestamp, but in
multi-query charts the last run timestamp is typically in the last array
element; also `queriesResponse` may be a single object in some code paths.
Replace the indexed access with code that safely handles arrays and
single-object responses and selects the last item when `queriesResponse` is an
array. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
const queriedDttm =
Array.isArray(queriesResponse) && queriesResponse.length > 0
? queriesResponse[queriesResponse.length - 1]?.queried_dttm ?? null
: queriesResponse?.queried_dttm ?? null;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current code unconditionally picks queriesResponse[0] which can be wrong
for multi-query charts where the last query's timestamp is in the last element.
The proposed replacement safely handles both arrays and single-object responses
and picks the last array element, which fixes an actual correctness issue
visible in this file.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx
**Line:** 315:315
**Comment:**
*Logic Error: Logic error: the code reads the first element of
`queriesResponse` (`[0]`) to determine the "last queried" timestamp, but in
multi-query charts the last run timestamp is typically in the last array
element; also `queriesResponse` may be a single object in some code paths.
Replace the indexed access with code that safely handles arrays and
single-object responses and selects the last item when `queriesResponse` is an
array.
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/common/utils/query_cache_manager.py:
##########
@@ -125,6 +129,7 @@ def set_query_result(
"rejected_filter_columns": self.rejected_filter_columns,
"annotation_data": self.annotation_data,
"sql_rowcount": self.sql_rowcount,
+ "queried_dttm": self.queried_dttm,
}
Review Comment:
**Suggestion:** Backwards compatibility bug: `get()` expects a `dttm`
fallback when `queried_dttm` is missing, but `set_query_result()` does not
populate `dttm`, so reads may fail or rely on exception handling; include a
`dttm` entry (set to the same value as `queried_dttm`) when writing the cache
to preserve the existing `dttm` contract. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
"dttm": self.queried_dttm,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The get() path still falls back to "dttm" when "queried_dttm" is missing.
set_query_result()
currently doesn't populate "dttm", which can break backward compatibility
with existing
cache readers. Adding "dttm": self.queried_dttm preserves the legacy
contract and avoids
surprises when older consumers or other code expect "dttm".
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/common/utils/query_cache_manager.py
**Line:** 133:133
**Comment:**
*Possible Bug: Backwards compatibility bug: `get()` expects a `dttm`
fallback when `queried_dttm` is missing, but `set_query_result()` does not
populate `dttm`, so reads may fail or rely on exception handling; include a
`dttm` entry (set to the same value as `queried_dttm`) when writing the cache
to preserve the existing `dttm` contract.
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/common/utils/query_cache_manager.py:
##########
@@ -108,6 +111,7 @@ def set_query_result(
self.df = query_result.df
self.sql_rowcount = query_result.sql_rowcount
self.annotation_data = {} if annotation_data is None else
annotation_data
+ self.queried_dttm =
datetime.now(tz=timezone.utc).isoformat().split(".")[0]
Review Comment:
**Suggestion:** Inconsistent and lossy timestamp formatting: using
.isoformat().split(".")[0] strips fractional seconds but also can drop timezone
information when microseconds are present, producing inconsistent timestamp
strings; generate a consistent, timezone-aware ISO timestamp without
microseconds instead. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
self.queried_dttm =
datetime.now(timezone.utc).replace(microsecond=0).isoformat()
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current expression strips fractional seconds by splitting on '.', but in
cases
where microseconds exist it also removes the timezone suffix (e.g. "+00:00"),
producing inconsistent values. Replacing microseconds via
replace(microsecond=0)
and then calling isoformat() yields a consistent timezone-aware ISO8601
string
without microseconds. This fixes a real correctness/consistency issue.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/common/utils/query_cache_manager.py
**Line:** 114:114
**Comment:**
*Logic Error: Inconsistent and lossy timestamp formatting: using
.isoformat().split(".")[0] strips fractional seconds but also can drop timezone
information when microseconds are present, producing inconsistent timestamp
strings; generate a consistent, timezone-aware ISO timestamp without
microseconds instead.
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/src/dashboard/components/gridComponents/Chart/Chart.jsx:
##########
@@ -310,10 +312,16 @@ const Chart = props => {
return DEFAULT_HEADER_HEIGHT;
}, [headerRef]);
+ const queriedDttm = queriesResponse?.[0]?.queried_dttm ?? null;
+
const getChartHeight = useCallback(() => {
const headerHeight = getHeaderHeight();
- return Math.max(height - headerHeight - descriptionHeight, 20);
- }, [getHeaderHeight, height, descriptionHeight]);
+ const queriedLabelHeight = queriedDttm ? QUERIED_LABEL_HEIGHT : 0;
Review Comment:
**Suggestion:** In `getChartHeight` the code uses `queriedDttm` in a truthy
check to deduct `QUERIED_LABEL_HEIGHT`, which is inconsistent with the
rendering fix above; use an explicit null/undefined check so the computed chart
height and the rendered label condition remain consistent (e.g., both treat 0
as a valid timestamp). [logic error]
**Severity Level:** Minor ⚠️
```suggestion
const queriedLabelHeight = queriedDttm != null ? QUERIED_LABEL_HEIGHT :
0;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The height calculation uses a truthy check on queriedDttm, so a valid
timestamp like 0 would both be omitted from rendering (currently) and
incorrectly excluded from height adjustments. Making this an explicit
null/undefined check keeps height computation consistent with rendering
semantics and fixes a subtle layout mismatch.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx
**Line:** 319:319
**Comment:**
*Logic Error: In `getChartHeight` the code uses `queriedDttm` in a
truthy check to deduct `QUERIED_LABEL_HEIGHT`, which is inconsistent with the
rendering fix above; use an explicit null/undefined check so the computed chart
height and the rendered label condition remain consistent (e.g., both treat 0
as a valid timestamp).
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/src/dashboard/components/gridComponents/Chart/Chart.jsx:
##########
@@ -717,6 +725,10 @@ const Chart = props => {
/>
</ChartWrapper>
+ {!isLoading && queriedDttm && (
Review Comment:
**Suggestion:** Boolean truthiness is used to decide whether to render
`LastQueriedLabel`, so a valid timestamp of 0 (epoch) or an empty string would
be treated as falsy and the label would be hidden; use an explicit
null/undefined check to correctly render valid but falsy timestamps. [possible
bug]
**Severity Level:** Critical 🚨
```suggestion
{!isLoading && queriedDttm != null && (
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Using a truthy check will incorrectly omit rendering for valid falsy
timestamps (e.g., 0). Changing the condition to an explicit null/undefined
check (queriedDttm != null) ensures valid timestamps are shown while still
skipping absent values.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx
**Line:** 728:728
**Comment:**
*Possible Bug: Boolean truthiness is used to decide whether to render
`LastQueriedLabel`, so a valid timestamp of 0 (epoch) or an empty string would
be treated as falsy and the label would be hidden; use an explicit
null/undefined check to correctly render valid but falsy timestamps.
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]
