codeant-ai-for-open-source[bot] commented on code in PR #36881:
URL: https://github.com/apache/superset/pull/36881#discussion_r2655719692
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:
##########
@@ -387,7 +387,7 @@ const AgGridDataTable: FunctionComponent<AgGridTableProps>
= memo(
<div className="search-container">
{serverPagination && (
<div className="search-by-text-container">
- <span className="search-by-text"> Search by :</span>
+ <span className="search-by-text"> {t('Search by')} :</span>
Review Comment:
**Suggestion:** Internationalization bug: the colon (":") is placed outside
the translated string and there's an extra leading space before the translated
text; in some languages the colon placement differs or spacing is significant,
so leaving the colon outside `t(...)` will make translation incorrect and the
visible leading space is unintended. Put the colon inside the translation and
remove the stray space so translators can position punctuation correctly.
[logic error]
**Severity Level:** Minor ⚠️
```suggestion
<span className="search-by-text">{t('Search by:')}</span>
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a valid i18n issue: punctuation and spacing can be
language-dependent, and the stray leading space will render oddly. Moving the
colon into the translation key lets translators place punctuation correctly.
Note: changing the translation key will require updating the translation
catalog/extraction so existing translations don't disappear.
</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-ag-grid-table/src/AgGridTable/index.tsx
**Line:** 390:390
**Comment:**
*Logic Error: Internationalization bug: the colon (":") is placed
outside the translated string and there's an extra leading space before the
translated text; in some languages the colon placement differs or spacing is
significant, so leaving the colon outside `t(...)` will make translation
incorrect and the visible leading space is unintended. Put the colon inside the
translation and remove the stray space so translators can position punctuation
correctly.
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-ag-grid-table/src/AgGridTable/index.tsx:
##########
@@ -525,11 +525,7 @@ const AgGridDataTable: FunctionComponent<AgGridTableProps>
= memo(
{serverPagination && (
<Pagination
currentPage={serverPaginationData?.currentPage || 0}
- pageSize={
- hasServerPageLengthChanged
- ? serverPageLength
- : serverPaginationData?.pageSize || 10
- }
+ pageSize={serverPaginationData?.pageSize ?? serverPageLength}
Review Comment:
**Suggestion:** Type/runtime bug: `serverPaginationData?.pageSize` can be a
string or undefined and is passed directly to `pageSize`; that may give the
Pagination component an invalid type or undefined value. Coerce the resolved
value to a number and provide a safe numeric fallback so the prop always
receives a number. [type error]
**Severity Level:** Minor ⚠️
```suggestion
pageSize={Number(serverPaginationData?.pageSize ??
serverPageLength ?? 0)}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
serverPaginationData is a JsonObject and pageSize may be a string or
undefined at runtime. Coercing to Number (with a safe fallback) prevents
passing a non-numeric value into Pagination which likely expects a number and
avoids runtime/typing issues.
</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-ag-grid-table/src/AgGridTable/index.tsx
**Line:** 528:528
**Comment:**
*Type Error: Type/runtime bug: `serverPaginationData?.pageSize` can be
a string or undefined and is passed directly to `pageSize`; that may give the
Pagination component an invalid type or undefined value. Coerce the resolved
value to a number and provide a safe numeric fallback so the prop always
receives 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/plugin-chart-table/src/TableChart.tsx:
##########
@@ -885,6 +886,7 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
// typing is incorrect in current version of `@types/react-table`
// so we ask TS not to check.
columnKey: key,
+ columnLabel: label,
Review Comment:
**Suggestion:** Logic bug: `columnLabel` is set to the original `label`
variable but the code computes and mutates `displayLabel` to account for
comparison columns; using `label` here means `columnLabel` will not reflect the
final displayed label (e.g., it won't pick up blanking or formatted display for
comparison columns). Replace the assignment to use `displayLabel` so
`columnLabel` matches what the header actually shows. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
columnLabel: displayLabel,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The function computes `displayLabel` to reflect the final visible header
(including special-casing comparison columns and blanking). The current code
returns `columnLabel: label` which will not match what the Header renders
(`displayLabel`). Changing to `columnLabel: displayLabel` fixes that logic
mismatch and will make downstream features (search options, UIs that show
column labels) consistent with the rendered header. This is a real behavioral
bugfix and safe because `displayLabel` is already defined in the same scope.
</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:** 889:889
**Comment:**
*Logic Error: Logic bug: `columnLabel` is set to the original `label`
variable but the code computes and mutates `displayLabel` to account for
comparison columns; using `label` here means `columnLabel` will not reflect the
final displayed label (e.g., it won't pick up blanking or formatted display for
comparison columns). Replace the assignment to use `displayLabel` so
`columnLabel` matches what the header actually shows.
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:
##########
@@ -561,7 +561,7 @@ export default typedMemo(function DataTable<D extends
object>({
<Flex wrap align="center" gap="middle">
{serverPagination && (
<Space size="small" className="search-select-container">
- <span className="search-by-label">Search by:</span>
+ <span className="search-by-label">{t('Search by')}:</span>
Review Comment:
**Suggestion:** Internationalization bug: the colon punctuation is appended
outside the translated string (`{t('Search by')}:`), which prevents translators
from handling punctuation correctly for other languages; include the colon
inside the translation call so the entire phrase (including punctuation) can be
localized. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
<span className="search-by-label">{t('Search by:')}</span>
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a valid i18n improvement. Keeping the colon outside the translated
string prevents translators from moving or removing punctuation for target
languages where punctuation rules differ. Including the colon in the
translation (e.g., t('Search by:')) allows full localization control and avoids
awkward translations.
</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:** 564:564
**Comment:**
*Possible Bug: Internationalization bug: the colon punctuation is
appended outside the translated string (`{t('Search by')}:`), which prevents
translators from handling punctuation correctly for other languages; include
the colon inside the translation call so the entire phrase (including
punctuation) can be localized.
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]
