codeant-ai-for-open-source[bot] commented on code in PR #37099:
URL: https://github.com/apache/superset/pull/37099#discussion_r2686717261
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx:
##########
@@ -381,7 +379,11 @@ export default typedMemo(function DataTable<D extends
object>({
) : null}
Review Comment:
**Suggestion:** Layout bug: the wrapper column for the page-size selector is
always rendered even when `hasPagination` is false, leaving an empty Bootstrap
column and breaking the grid alignment; move the wrapper inside the
`hasPagination` conditional so the column only exists when the selector is
shown. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
{hasPagination ? (
<div className="col-sm-1">
<SelectPageSize
total={resultsSize}
current={resultCurrentPageSize}
options={pageSizeOptions}
selectRenderer={
typeof selectPageSize === 'boolean'
? undefined
: selectPageSize
}
onChange={setPageSize}
/>
</div>
) : null}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The PR currently always renders a Bootstrap column wrapper (<div
className="col-sm-1">)
even when hasPagination is false, leaving an empty grid column. Moving the
wrapper
inside the hasPagination conditional fixes real layout misalignment. This is
a
functional bug fix, small and scoped to this hunk.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx
**Line:** 366:379
**Comment:**
*Logic Error: Layout bug: the wrapper column for the page-size selector
is always rendered even when `hasPagination` is false, leaving an empty
Bootstrap column and breaking the grid alignment; move the wrapper inside the
`hasPagination` conditional so the column only exists when the selector is
shown.
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/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -639,6 +638,18 @@ export default function transformProps(
focusedRow = rows.length - 1;
}
});
+ const { seriesName } = params;
+ if (seriesName) {
+ const regex = new RegExp('.*(\\d+) (\\w+) ago');
Review Comment:
**Suggestion:** The regex `new RegExp('.*(\\d+) (\\w+) ago')` is greedy and
unanchored and can match unintended parts of the series name; use an anchored,
whitespace-tolerant, case-insensitive regex to reliably capture the trailing "N
unit ago" pattern. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
const regex = /(\d+)\s+(\w+)\s+ago$/i;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current regex is unanchored and greedy, which can match unintended
digits earlier in longer series names. Replacing it with an anchored,
whitespace-tolerant, case-insensitive pattern that targets a trailing "N unit
ago" is more accurate and reduces false matches.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
**Line:** 643:643
**Comment:**
*Logic Error: The regex `new RegExp('.*(\\d+) (\\w+) ago')` is greedy
and unanchored and can match unintended parts of the series name; use an
anchored, whitespace-tolerant, case-insensitive regex to reliably capture the
trailing "N unit ago" pattern.
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/plugin-chart-table/src/TableChart.tsx:
##########
@@ -571,7 +576,7 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
color: ${theme.colors.grayscale.dark2};
`}
>
- {column.label}
+ {t(column.label)}
Review Comment:
**Suggestion:** In the dropdown menu you call the translator on
`column.label` (`t(column.label)`), but `column.label` is already the
user-facing label (likely already localized). Translating it again can cause
incorrect or double translations and unstable display; use the label as-is to
reliably display the column name. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
{column.label}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
column.label in this dropdown context is already the display label; passing
it into the translator again risks double-translation or lookup failures.
Rendering the label directly is the correct, stable behavior for UI text
that has already been localized at source.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx
**Line:** 579:579
**Comment:**
*Possible Bug: In the dropdown menu you call the translator on
`column.label` (`t(column.label)`), but `column.label` is already the
user-facing label (likely already localized). Translating it again can cause
incorrect or double translations and unstable display; use the label as-is to
reliably display the column name.
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/scripts/lokalise_merger.sh:
##########
@@ -0,0 +1,19 @@
+#!/bin/bash
+
+set -e
+
+for file in $( find ../superset/translations/** -name '*.json' );
+do
+ extension=${file##*.}
+ filename="${file%.*}"
+ if [ $extension == "json" ]
Review Comment:
**Suggestion:** Unquoted variable in the test and use of '==' can lead to
test failures or word-splitting when `extension` is empty or contains spaces;
quote the variable and use '=' (POSIX) or [[ ... ]] to make the comparison
robust. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
if [ "$extension" = "json" ]
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Leaving $extension unquoted is dangerous — if it's empty or contains
whitespace the test can fail or produce a syntax error. Quoting it (and using =
for POSIX compatibility) is the robust fix. Note: the shebang is bash so '=='
works, but the unquoted variable is the real bug.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/scripts/lokalise_merger.sh
**Line:** 9:9
**Comment:**
*Logic Error: Unquoted variable in the test and use of '==' can lead to
test failures or word-splitting when `extension` is empty or contains spaces;
quote the variable and use '=' (POSIX) or [[ ... ]] to make the comparison
robust.
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>
##########
Dockerfile:
##########
@@ -66,26 +66,19 @@ RUN
--mount=type=bind,source=./superset-frontend/package.json,target=./package.j
# Runs the webpack build process
COPY superset-frontend /app/superset-frontend
-# Build the frontend if not in dev mode
-RUN --mount=type=cache,target=/app/superset-frontend/.temp_cache \
- --mount=type=cache,target=/root/.npm \
- if [ "$DEV_MODE" = "false" ]; then \
- echo "Running 'npm run ${BUILD_CMD}'"; \
- npm run ${BUILD_CMD}; \
- else \
- echo "Skipping 'npm run ${BUILD_CMD}' in dev mode"; \
- fi;
-
-# Copy translation files
+# This copies the .po files needed for translation
+RUN mkdir -p /app/superset/translations
COPY superset/translations /app/superset/translations
-# Build the frontend if not in dev mode
-RUN if [ "$BUILD_TRANSLATIONS" = "true" ]; then \
- npm run build-translation; \
- fi; \
- rm -rf /app/superset/translations/*/*/*.po; \
- rm -rf /app/superset/translations/*/*/*.mo;
+RUN mkdir -p /app/locales
+COPY locales /app/locales
+
+# Compiles .json files from the .po files, then deletes the .po files
+RUN npm run build-translation
Review Comment:
**Suggestion:** The translation build is run unconditionally; when
`BUILD_TRANSLATIONS` is set to false (or translations are not present) `npm run
build-translation` will still run and can fail or waste build time — guard the
step with the `BUILD_TRANSLATIONS` flag so it only runs when requested. [logic
error]
**Severity Level:** Minor ⚠️
```suggestion
RUN if [ "${BUILD_TRANSLATIONS}" = "true" ]; then \
npm run build-translation; \
else \
echo "Skipping translation build (BUILD_TRANSLATIONS != true)"; \
fi
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Guarding the translation build with BUILD_TRANSLATIONS is reasonable:
running npm run build-translation when translations are disabled wastes time
and can fail in edge cases. The suggestion correctly adds a conditional around
the RUN step.
Note: the COPY of superset/translations earlier in this stage can still fail
if sources are absent, so the guard alone doesn't fully eliminate all failure
modes.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** Dockerfile
**Line:** 77:77
**Comment:**
*Logic Error: The translation build is run unconditionally; when
`BUILD_TRANSLATIONS` is set to false (or translations are not present) `npm run
build-translation` will still run and can fail or waste build time — guard the
step with the `BUILD_TRANSLATIONS` flag so it only runs when requested.
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/plugin-chart-table/src/DataTable/DataTable.tsx:
##########
@@ -381,7 +379,11 @@ export default typedMemo(function DataTable<D extends
object>({
) : null}
</div>
{searchInput ? (
- <div className="col-sm-6">
+ <div
+ className={
+ renderTimeComparisonDropdown ? 'col-sm-10' : 'col-sm-11'
Review Comment:
**Suggestion:** Incorrect width calculation for the search input column: the
new className only checks `renderTimeComparisonDropdown` and ignores whether
the pagination column is present, resulting in mis-sized columns in
combinations of pagination and dropdown presence; compute the column size using
both `hasPagination` and `renderTimeComparisonDropdown`. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
hasPagination
? renderTimeComparisonDropdown
? 'col-sm-10'
: 'col-sm-11'
: renderTimeComparisonDropdown
? 'col-sm-11'
: 'col-sm-12'
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current className only considers renderTimeComparisonDropdown and
ignores whether
the pagination column (col-sm-1) is present, which can produce incorrect
column widths.
The proposed mapping using both hasPagination and
renderTimeComparisonDropdown correctly
accounts for all combinations so the 12-column grid sums up properly. This
resolves a
visible layout 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/plugin-chart-table/src/DataTable/DataTable.tsx
**Line:** 384:384
**Comment:**
*Logic Error: Incorrect width calculation for the search input column:
the new className only checks `renderTimeComparisonDropdown` and ignores
whether the pagination column is present, resulting in mis-sized columns in
combinations of pagination and dropdown presence; compute the column size using
both `hasPagination` and `renderTimeComparisonDropdown`.
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/plugin-chart-table/src/TableChart.tsx:
##########
@@ -270,9 +270,9 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
} = props;
const comparisonColumns = [
{ key: 'all', label: t('Display all') },
- { key: '#', label: '#' },
- { key: '△', label: '△' },
- { key: '%', label: '%' },
+ { key: t('sv_previous'), label: t('sv_previous') },
+ { key: t('sv_change'), label: t('sv_change') },
+ { key: t('sv_change_percentage'), label: t('sv_change_percentage') },
Review Comment:
**Suggestion:** Logic bug: using translated strings (result of `t(...)`) as
stable keys for `comparisonColumns`. Translation output can change across
locales and is not a stable identifier; this will break comparisons, lookups
and selection logic that expects stable keys (for example,
`selectedComparisonColumns.includes(column.key)` and substring operations on
`key`). Use stable, locale-independent keys (raw identifiers) and keep `label`
as the translated string. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
{ key: 'sv_previous', label: t('sv_previous') },
{ key: 'sv_change', label: t('sv_change') },
{ key: 'sv_change_percentage', label: t('sv_change_percentage') },
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a real logic bug: keys are used as identifiers (Menu.Item keys,
selection state, and includes/substrings elsewhere). Using the translation
output as the key makes identifiers locale-dependent and unstable across
runs/locales. Replacing the key with a stable identifier (and keeping the
translated label for display) fixes that class of bugs.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx
**Line:** 273:275
**Comment:**
*Logic Error: Logic bug: using translated strings (result of `t(...)`)
as stable keys for `comparisonColumns`. Translation output can change across
locales and is not a stable identifier; this will break comparisons, lookups
and selection logic that expects stable keys (for example,
`selectedComparisonColumns.includes(column.key)` and substring operations on
`key`). Use stable, locale-independent keys (raw identifiers) and keep `label`
as the translated string.
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/plugin-chart-table/src/TableChart.tsx:
##########
@@ -616,11 +621,10 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
/>,
);
}
-
// Add the current header <th>
headers.push(
<th key={`header-${key}`} colSpan={colSpan} style={{ borderBottom: 0
}}>
Review Comment:
**Suggestion:** In the grouping header you call `t(key.trim())` where `key`
is a grouping key (not a translation key). Passing arbitrary internal keys into
the translator risks incorrect lookups and unexpected output; render the
trimmed key directly unless `key` is a known translation key. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
{key.trim()}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The grouping `key` originates from internal logic (derived keys/group ids),
not user-facing translation keys. Sending internal identifiers to the
translator can yield incorrect output or missing translations. Rendering
the trimmed key directly (or mapping internal keys to explicit translation
keys) is the correct approach.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx
**Line:** 626:626
**Comment:**
*Possible Bug: In the grouping header you call `t(key.trim())` where
`key` is a grouping key (not a translation key). Passing arbitrary internal
keys into the translator risks incorrect lookups and unexpected output; render
the trimmed key directly unless `key` is a known translation key.
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/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -580,9 +581,7 @@ export default function transformProps(
show: !inContextMenu,
trigger: richTooltip ? 'axis' : 'item',
formatter: (params: any) => {
- const xValue: number = richTooltip
- ? params[0].value[0]
- : params.value[0];
+ let xValue: number = richTooltip ? params[0].value[0] :
params.value[0];
Review Comment:
**Suggestion:** Accessing `params[0].value[0]` or `params.value[0]` directly
can throw if `params` or `params[0]` is undefined or doesn't have the expected
shape; use optional chaining and explicit numeric coercion to avoid TypeError
and ensure `xValue` is a number. [null pointer]
**Severity Level:** Minor ⚠️
```suggestion
const rawX = richTooltip ? params?.[0]?.value?.[0] :
params?.value?.[0];
let xValue: number = rawX != null ? Number(rawX) : NaN;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The original line does assume the echarts `params` shape and will throw if
`params` or `params[0]` are missing or unexpectedly shaped. Guarding with
optional chaining and coercing to Number is a sensible defensive change to
avoid runtime TypeErrors inside the tooltip formatter, which runs in user
interactions. The suggested change is relevant and improves robustness.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
**Line:** 584:584
**Comment:**
*Null Pointer: Accessing `params[0].value[0]` or `params.value[0]`
directly can throw if `params` or `params[0]` is undefined or doesn't have the
expected shape; use optional chaining and explicit numeric coercion to avoid
TypeError and ensure `xValue` 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>
##########
Dockerfile:
##########
@@ -66,26 +66,19 @@ RUN
--mount=type=bind,source=./superset-frontend/package.json,target=./package.j
# Runs the webpack build process
COPY superset-frontend /app/superset-frontend
-# Build the frontend if not in dev mode
-RUN --mount=type=cache,target=/app/superset-frontend/.temp_cache \
- --mount=type=cache,target=/root/.npm \
- if [ "$DEV_MODE" = "false" ]; then \
- echo "Running 'npm run ${BUILD_CMD}'"; \
- npm run ${BUILD_CMD}; \
- else \
- echo "Skipping 'npm run ${BUILD_CMD}' in dev mode"; \
- fi;
-
-# Copy translation files
+# This copies the .po files needed for translation
+RUN mkdir -p /app/superset/translations
COPY superset/translations /app/superset/translations
-# Build the frontend if not in dev mode
-RUN if [ "$BUILD_TRANSLATIONS" = "true" ]; then \
- npm run build-translation; \
- fi; \
- rm -rf /app/superset/translations/*/*/*.po; \
- rm -rf /app/superset/translations/*/*/*.mo;
+RUN mkdir -p /app/locales
+COPY locales /app/locales
+
+# Compiles .json files from the .po files, then deletes the .po files
+RUN npm run build-translation
+RUN rm /app/superset/translations/*/LC_MESSAGES/*.po
+RUN rm /app/superset/translations/messages.pot
Review Comment:
**Suggestion:** The `rm` commands will fail the build if the files/globs
don't exist because they are not using force-mode; use `rm -f` so the Docker
build doesn't fail when translations aren't present. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
RUN rm -f /app/superset/translations/*/LC_MESSAGES/*.po
RUN rm -f /app/superset/translations/messages.pot
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Using rm -f here is a pragmatic, minimal change that prevents a
non-existent-glob from failing the Docker build. It's safe and improves
robustness when translations are absent.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** Dockerfile
**Line:** 78:79
**Comment:**
*Possible Bug: The `rm` commands will fail the build if the files/globs
don't exist because they are not using force-mode; use `rm -f` so the Docker
build doesn't fail when translations aren't present.
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/config.py:
##########
@@ -393,7 +393,14 @@ def _try_json_readsha(filepath: str, length: int) -> str |
None:
}
# Turning off i18n by default as translation in most languages are
# incomplete and not well maintained.
-LANGUAGES = {}
+LANGUAGES = {
+ "en": {"flag": "us", "name": "English"},
+ "es": {"flag": "es", "name": "Spanish"},
+ "it": {"flag": "it", "name": "Italian"},
+ "fr": {"flag": "fr", "name": "French"},
+ "de": {"flag": "de", "name": "German"},
+ "nl": {"flag": "nl", "name": "Dutch"},
+}
Review Comment:
**Suggestion:** Duplicate `LANGUAGES` definition: the new small `LANGUAGES`
dict overwrites or conflicts with the larger `LANGUAGES` mapping defined later
in the file, causing loss of many language entries and unexpected behavior.
Remove the duplicate definition or defer/merge it with the main mapping to
avoid shadowing. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
# LANGUAGES is defined later in this file; do not redefine it here to avoid
# accidentally overwriting the comprehensive LANGUAGES mapping.
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This suggestion is correct: the PR added a small LANGUAGES dict that
immediately comes after an existing, much larger LANGUAGES mapping in the file,
so the later smaller definition will shadow and overwrite the comprehensive
mapping, causing loss of many locales. Removing or merging the duplicate is a
real logic bugfix, not a cosmetic nit.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/config.py
**Line:** 396:403
**Comment:**
*Logic Error: Duplicate `LANGUAGES` definition: the new small
`LANGUAGES` dict overwrites or conflicts with the larger `LANGUAGES` mapping
defined later in the file, causing loss of many language entries and unexpected
behavior. Remove the duplicate definition or defer/merge it with the main
mapping to avoid shadowing.
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/plugin-chart-table/src/TableChart.tsx:
##########
@@ -966,7 +970,7 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
alignItems: 'flex-end',
}}
>
- <span data-column-name={col.id}>{label}</span>
+ <span data-column-name={col.id}>{t(label)}</span>
Review Comment:
**Suggestion:** In the column header rendering you call `t(label)` even
though `label` is the column's display label (already the user-facing value).
Translating `label` again risks double translation or missing translations;
render `label` directly to ensure stable header text. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
<span data-column-name={col.id}>{label}</span>
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Column labels are already the user-facing strings; wrapping them again with
t() risks double translation or lookup misses. Using the label directly
ensures stable header text and avoids accidental mangling by the
translation layer.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx
**Line:** 973:973
**Comment:**
*Possible Bug: In the column header rendering you call `t(label)` even
though `label` is the column's display label (already the user-facing value).
Translating `label` again risks double translation or missing translations;
render `label` directly to ensure stable header text.
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/plugin-chart-table/src/transformProps.ts:
##########
@@ -118,21 +118,27 @@ const processComparisonTotals = (
totals.map((totalRecord: DataRecord) =>
Object.keys(totalRecord).forEach(key => {
if (totalRecord[key] !== undefined && !key.includes(comparisonSuffix)) {
- transformedTotals[`Main ${key}`] =
- parseInt(transformedTotals[`Main ${key}`]?.toString() || '0', 10) +
- parseInt(totalRecord[key]?.toString() || '0', 10);
- transformedTotals[`# ${key}`] =
- parseInt(transformedTotals[`# ${key}`]?.toString() || '0', 10) +
+ transformedTotals[`${t('sv_current')} ${key}`] =
+ parseInt(
+ transformedTotals[`${t('sv_current')} ${key}`]?.toString() || '0',
+ 10,
+ ) + parseInt(totalRecord[key]?.toString() || '0', 10);
+ transformedTotals[`${t('sv_previous')} ${key}`] =
+ parseInt(
+ transformedTotals[`${t('sv_previous')} ${key}`]?.toString() || '0',
+ 10,
+ ) +
parseInt(
totalRecord[`${key}__${comparisonSuffix}`]?.toString() || '0',
10,
);
const { valueDifference, percentDifferenceNum } = calculateDifferences(
- transformedTotals[`Main ${key}`] as number,
- transformedTotals[`# ${key}`] as number,
+ transformedTotals[`${t('sv_current')} ${key}`] as number,
Review Comment:
**Suggestion:** Using parseInt to aggregate numeric totals will truncate
decimal/fractional values (e.g. "12.34" -> 12). This causes incorrect totals
and percentage/change calculations for non-integer metrics; use Number() or
parseFloat() to preserve decimals. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
Number(
transformedTotals[`${t('sv_current')} ${key}`]?.toString() || '0',
) + Number(totalRecord[key]?.toString() || '0');
transformedTotals[`${t('sv_previous')} ${key}`] =
Number(
transformedTotals[`${t('sv_previous')} ${key}`]?.toString() || '0',
) +
Number(
totalRecord[`${key}__${comparisonSuffix}`]?.toString() || '0',
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a real logic bug: parseInt drops fractional parts so totals and
percent
diffs for floating metrics will be wrong. Switching to Number() or
parseFloat()
preserves decimals and fixes the inaccuracy. The suggested improved_code
directly
targets the offending lines.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-table/src/transformProps.ts
**Line:** 119:136
**Comment:**
*Logic Error: Using parseInt to aggregate numeric totals will truncate
decimal/fractional values (e.g. "12.34" -> 12). This causes incorrect totals
and percentage/change calculations for non-integer metrics; use Number() or
parseFloat() to preserve decimals.
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]
