fix(table charts v1 & v2): pagination, translation and search by #36881
Conversation
…ame as table v2 plugin)
…tion-search_by_label
|
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 · |
Code Review Agent Run #76bb8fActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
codeant-ai-for-open-source
bot
added
the
size:XS
Nitpicks 🔍
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| {serverPagination && ( | ||
| <div className="search-by-text-container"> | ||
| <span className="search-by-text"> Search by :</span> | ||
| <span className="search-by-text"> {t('Search by')} :</span> |
There was a problem hiding this 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
| <span className="search-by-text"> {t('Search by')} :</span> | |
| <span className="search-by-text">{t('Search by:')}</span> |
Why it matters? ⭐
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.
Prompt for AI Agent 🤖
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.| ? serverPageLength | ||
| : serverPaginationData?.pageSize || 10 | ||
| } | ||
| pageSize={serverPaginationData?.pageSize ?? serverPageLength} |
There was a problem hiding this 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
| pageSize={serverPaginationData?.pageSize ?? serverPageLength} | |
| pageSize={Number(serverPaginationData?.pageSize ?? serverPageLength ?? 0)} |
Why it matters? ⭐
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.
Prompt for AI Agent 🤖
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.| {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> |
There was a problem hiding this 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 🚨
| <span className="search-by-label">{t('Search by')}:</span> | |
| <span className="search-by-label">{t('Search by:')}</span> |
Why it matters? ⭐
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.
Prompt for AI Agent 🤖
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.| // typing is incorrect in current version of `@types/react-table` | ||
| // so we ask TS not to check. | ||
| columnKey: key, | ||
| columnLabel: label, |
There was a problem hiding this 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
| columnLabel: label, | |
| columnLabel: displayLabel, |
Why it matters? ⭐
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.
Prompt for AI Agent 🤖
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.
|
CodeAnt AI finished reviewing your PR. |
rusackas
changed the title
rusackas
changed the title
1 participant
User description
User description
SUMMARY
Fixing several issues with "Table" and "Table V2" plugins.
"Table":
When server-side pagination is used, a "Search by" selector is displayed: it lists columns by their name rather than by their label, unlike the "Table V2" chart. I therefore made the necessary changes so that labels are used.
"Table V2":
When server-side pagination is used, the "Page Size" selector does not behave as expected: the value configured for the chart is not applied correctly and the default value is applied (it is fine on initial display but is lost afterwards, for example when resizing the window).
"Table" and "Table V2":
Some strings could not be translated because the "t()" function intended for this purpose was not used.
CodeAnt-AI Description
Use column labels for "Search by", persist page size, and add translations for table charts
What Changed
Impact
✅ Clearer search labels in table filters✅ Page size persists across interactions✅ Localized table UI strings💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.