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: [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]
