You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "GregLebret (via GitHub)" <gi...@apache.org> on 2026/01/13 14:31:34 UTC
[PR] Sup 130 [superset]
GregLebret opened a new pull request, #37099:
URL: https://github.com/apache/superset/pull/37099
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Sup 130 [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #37099:
URL: https://github.com/apache/superset/pull/37099#discussion_r2686759630
##########
superset-frontend/scripts/lokalise_merger.sh:
##########
@@ -0,0 +1,19 @@
+#!/bin/bash
+
+set -e
+
+for file in $( find ../superset/translations/** -name '*.json' );
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unsafe file iteration with spaces</b></div>
<div id="fix">
The find command uses ** which relies on globstar being enabled for recursion, and the for loop with command substitution splits on spaces in filenames, potentially missing files or failing on paths with spaces. This could lead to incomplete translation merging.
</div>
</div>
<small><i>Code Review Run #6ea781</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect conditional logic</b></div>
<div id="fix">
The `npm run build-translation` command is now executed unconditionally, but it should be conditional on the `BUILD_TRANSLATIONS` environment variable to avoid building translations when they are not intended to be included in the image, as per the project's documentation and previous logic.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
RUN if [ "$BUILD_TRANSLATIONS" = "true" ]; then npm run build-translation; fi;
````
</div>
</details>
</div>
<small><i>Code Review Run #6ea781</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx:
##########
@@ -22,7 +22,7 @@ import {
InPortal,
OutPortal,
} from 'react-reverse-portal';
-import { styled, SupersetTheme, truncationCSS } from '@superset-ui/core';
+import { t, styled, SupersetTheme, truncationCSS } from '@superset-ui/core';
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect i18n on user data</b></div>
<div id="fix">
The change adds translation calls to filter.name and filter.description, but these are user-entered values from the Filter type, not i18n keys. Wrapping them with t() will attempt to translate user data, leading to incorrect display. Other usages in the codebase, like in NameRow.tsx and CrossFilter.tsx, display filter.name directly without t().
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
- import { t, styled, SupersetTheme, truncationCSS } from '@superset-ui/core';
+ import { styled, SupersetTheme, truncationCSS } from '@superset-ui/core';
@@ -259,1 +259,1 @@
- {t(name)}
+ {name}
@@ -263,1 +263,1 @@
- <DescriptionToolTip description={t(filter.description)} />
+ <DescriptionToolTip description={filter.description} />
```
</div>
</details>
</div>
<small><i>Code Review Run #6ea781</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/views/base.py:
##########
@@ -348,8 +349,11 @@ def cached_common_bootstrap_data( # pylint: disable=unused-argument
def common_bootstrap_payload() -> dict[str, Any]:
+ locale = get_locale()
+ if request.args.get("lang"):
+ locale = Locale.parse(request.args.get("lang"))
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unhandled exception in locale parsing</b></div>
<div id="fix">
Wrap the call to `Locale.parse(request.args.get("lang"))` in a try/except catching `UnknownLocaleError`, `ValueError`, and `TypeError` so that invalid locale strings fall back to the default locale instead of raising 500 errors.
</div>
</div>
<small><i>Code Review Run #6ea781</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/components/EditableTitle/index.tsx:
##########
@@ -232,10 +232,10 @@ export default function EditableTitle({
display: inline-block;
`}
>
- {value}
+ {t(value)}
</Link>
) : (
- <span data-test="editable-title-input">{value}</span>
+ <span data-test="editable-title-input">{t(value)}</span>
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect i18n on user input</b></div>
<div id="fix">
Similar to line 235, the span also incorrectly translates the user-editable value.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
<span data-test="editable-title-input">{value}</span>
````
</div>
</details>
</div>
<small><i>Code Review Run #6ea781</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/components/EditableTitle/index.tsx:
##########
@@ -232,10 +232,10 @@ export default function EditableTitle({
display: inline-block;
`}
>
- {value}
+ {t(value)}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect i18n on user input</b></div>
<div id="fix">
The title value, which can be user-edited, is being wrapped with t() for translation, but this treats user input as a translation key, which is incorrect and may alter display unexpectedly.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
{value}
````
</div>
</details>
</div>
<small><i>Code Review Run #6ea781</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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" ]
+ then
+ locale=${file/#..\/superset\/translations\/}
+ locale=${locale%\/LC_MESSAGES\/messages.json}
+ if [ -f "../locales/$locale/translation.json" ]
+ then
+ output=$(jq -s '.[0].locale_data.superset *= (.[1] | with_entries({key:.key,value:[.value]})) | first' $file "../locales/$locale/translation.json")
+ echo $output > $file
+ fi
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Word splitting corrupts JSON output</b></div>
<div id="fix">
The echo command lacks quotes around $output, causing bash to perform word splitting on the JSON string, which corrupts the output file when the JSON contains spaces. This changes observable behavior by producing invalid JSON.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
output=$(jq -s '.[0].locale_data.superset *= (.[1] | with_entries({key:.key,value:[.value]})) | first' $file "../locales/$locale/translation.json")
echo "$output" > $file
fi
````
</div>
</details>
</div>
<small><i>Code Review Run #6ea781</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Sup 130 [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37099:
URL: https://github.com/apache/superset/pull/37099#issuecomment-3744666336
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Sup 130 [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #37099:
URL: https://github.com/apache/superset/pull/37099#issuecomment-3744665834
CodeAnt AI is reviewing your PR.
---
### Thanks for using CodeAnt! ๐
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) ยท
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) ยท
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Sup 130 [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #37099:
URL: https://github.com/apache/superset/pull/37099#issuecomment-3744707193
CodeAnt AI finished reviewing your PR.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Sup 130 [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #37099:
URL: https://github.com/apache/superset/pull/37099#issuecomment-3744687456
### <span aria-hidden="true">โ
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">๐จ</span> Latest commit | 81acbff2caae630f4d1d2a401e97f5674707f5eb |
|<span aria-hidden="true">๐</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/696657476ebefb0008698a51 |
|<span aria-hidden="true">๐</span> Deploy Preview | [https://deploy-preview-37099--superset-docs-preview.netlify.app](https://deploy-preview-37099--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">๐ฑ</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-37099--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Sup 130 [superset]
Posted by "GregLebret (via GitHub)" <gi...@apache.org>.
GregLebret closed pull request #37099: Sup 130
URL: https://github.com/apache/superset/pull/37099
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Sup 130 [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org